-
Notifications
You must be signed in to change notification settings - Fork 570
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Style Configuration Tool RFC #73
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here is a gif to give a little context on what this does.
I think this makes a great addition to the demoes 👍
If you can cleanup the amount of repeated code, that would be great. I have a feeling we can get this down to about 1000 lines. That would make it a little easier to give better feedback, as I'll have less lines to read.
Once you're satisfied, we can take another look at this and get it merged :)
demo/sdl_opengl3/main.c
Outdated
@@ -70,7 +75,7 @@ | |||
* DEMO | |||
* | |||
* ===============================================================*/ | |||
int main(int argc, char *argv[]) | |||
int main(void) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you have any problems with these arguments since you removed them? I vaguely remember someone adding these arguments to fix some problem they had.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was accidental. It was void up until 9 days ago and I started working on this long before that and just copied the version I had over after pulling. I'll fix it.
demo/style_configurator.c
Outdated
nk_layout_row_dynamic(ctx, 25, 1); | ||
colorf.r = nk_propertyf(ctx, "#R:", 0, colorf.r, 1.0f, 0.01f,0.005f); | ||
colorf.g = nk_propertyf(ctx, "#G:", 0, colorf.g, 1.0f, 0.01f,0.005f); | ||
colorf.b = nk_propertyf(ctx, "#B:", 0, colorf.b, 1.0f, 0.01f,0.005f); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be repeated a lot. A style_rgb_sliders
would probably be a good function to add.
demo/style_configurator.c
Outdated
nk_layout_row_dynamic(ctx, 25, 1); | ||
nk_property_float(ctx, "#X:", -100.0f, &text.padding.x, 100.0f, 1,0.5f); | ||
nk_property_float(ctx, "#Y:", -100.0f, &text.padding.y, 100.0f, 1,0.5f); | ||
nk_combo_end(ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. style_pointer_sliders
would be a good function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, like I said, this was definitely a first draft and I did a lot of copy-pasting and was mostly focused on just getting it to work and exploring the very large style hierarchy. I tend to follow something akin to Casey Muratori's "Semantic Compression" and posted this without any additional "passes". I'm not sure about hitting 1000 lines though, we'll see. I kind of want to keep the comments in there even though they're just pasted from nuklear.h; they're a quick reference for all the elements each function can change and those alone are a couple hundred lines. I'll try to finish up these changes today or tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. Do what you think needs to be done 👍
I'm not 100% done but it looks like you were almost exactly right. Even without getting rid of the comments I could probably get it down to an even 1000. And really I'm on the fence about the comments, they definitely stand out more in a semi-bad way now that it's shorter and cleaner. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heh, the 1000 lines was a lucky guess. Anyways, now that this PR is a little more approachable I've taken some time to review your code in a little more depth. Most of these things are nitpicks that are not blocking on this getting merged, but I've left one comment that shows off what I assumes to be unintended behavior in the code.
demo/style_configurator.c
Outdated
style_text(struct nk_context* ctx, struct nk_style_text* out_style) | ||
{ | ||
struct nk_style_text text = *out_style; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see you have this pattern where all these style_*
functions take an output parameter, but then the first line copies the parameter and the last line copies it back. Any specific reason for this? I'd personally prefer you either just don't take this copy and just pass pointers to the fields of out_style
or pass out_style
by value and return a new one. Here is how this would look:
// Pass mutable ptr
static void
style_text(struct nk_context* ctx, struct nk_style_text* out_style)
{
nk_layout_row_dynamic(ctx, 30, 2);
style_rgb(ctx, "Color:", &out_style.color);
style_vec2(ctx, "Padding:", &out_style.padding);
}
// Pass by value
static struct nk_style_text
style_text(struct nk_context* ctx, struct nk_style_text style)
{
nk_layout_row_dynamic(ctx, 30, 2);
style_rgb(ctx, "Color:", &style.color);
style_vec2(ctx, "Padding:", &style.padding);
return style;
}
I think the pass by pointer is preferred as it will be more efficient in debug builds and with less optimizing compilers. Either is fine though, as this is just demo code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I agree the pointer is better, though I'll definitely change the name of the parameters to match the current local style variables. I think my reasoning at the time was (as usual) laziness, and knowing I'd probably change it later. Though I honestly don't think it'd make a noticeable performance difference unless you were using unoptimized tcc builds on some low powered ARM core or something. I'm continually amazed at how fast even Raspberry Pi's etc. are these days.
Wait isn't your first example wrong? That's why I was too lazy to do it at first? Shouldn't it be
static void
style_text(struct nk_context* ctx, struct nk_style_text* out_style)
{
nk_layout_row_dynamic(ctx, 30, 2);
style_rgb(ctx, "Color:", &out_style->color);
style_vec2(ctx, "Padding:", &out_style->padding);
}
So my final version will be
static void
style_text(struct nk_context* ctx, struct nk_style_text* text)
{
nk_layout_row_dynamic(ctx, 30, 2);
style_rgb(ctx, "Color:", &text->color);
style_vec2(ctx, "Padding:", &text->padding);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, my example was incorrect. Didn't actually compile them :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Np, you can tell I didn't even notice till I was halfway through typing my response.
I've often heard/thought that the . vs -> is the one thing thing that the compiler could actually just issue a warning and fix automatically without any potential downsides. Or get rid of -> entirely but I disagree with that since I like the visual reminder something is a pointer.
And clearly the overhead of typing an extra character is enough that I often do this copying to a local variable thing.
demo/style_configurator.c
Outdated
//button->border = 1.0f; | ||
//button->rounding = 4.0f; | ||
//button->draw_begin = 0; | ||
//button->draw_end = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't think these comments add much. A person can easily just look at the declaration of struct nk_style_button
for this information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the other reason for leaving them in was it shows what the default values are but yeah they could still just look at nk_style_from_table() for that. I'll remove them.
demo/style_configurator.c
Outdated
} | ||
} | ||
|
||
// TODO style_style_item? how to handle images if at all? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that would either require a file browser or at least a predefined list of images to choose from. I think the latter is more in scope for this demo if you actually want to implement this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well to be useful in general I'd have to allow the user to specify the images so predefined either at compile or runtime by them and passed in somehow. The problem is I've never even looked at an example that uses images for style items. Do you know a good reference project/example that uses them?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, there is example/skinning.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
D'oh! I knew I should have looked before I asked. I remember seeing that when I first learned about Nuklear but since then I've always played around with the demos and never the examples. I'll have to spend some time going over all the examples again and maybe see what happens when I combine the configurator in its current form with them (probably breaks completely).
demo/style_configurator.c
Outdated
NK_TEXT_RIGHT = NK_TEXT_ALIGN_MIDDLE|NK_TEXT_ALIGN_RIGHT | ||
}; | ||
*/ | ||
// TODO support combining with TOP/MIDDLE/BOTTOM .. separate combo? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two combo boxes could be fine, but if it is simpler to implement by just listing out all combinations in one combo box I think that could work too. There are only 9 different combinations after all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Single does seem easier.
demo/style_configurator.c
Outdated
if (slider.show_buttons) { | ||
nk_layout_row_dynamic(ctx, 30, 2); | ||
nk_label(ctx, "Inc Symbol:", NK_TEXT_LEFT); | ||
slider.inc_symbol = nk_combo(ctx, symbols, NK_SYMBOL_MAX, slider.inc_symbol, 25, nk_vec2(200,200)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed this yesterday too and I'm not sure what's going on. I'll see which commit broke at try to figure it out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was my bad. See #79
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well that's a relief. I'm glad I hadn't gotten around to it yet.
demo/style_configurator.c
Outdated
slider.dec_symbol = nk_combo(ctx, symbols, NK_SYMBOL_MAX, slider.dec_symbol, 25, nk_vec2(200,200)); | ||
|
||
// necessary or do tree's always take the whole width? | ||
//nk_layout_row_dynamic(ctx, 30, 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
demo/style_configurator.c
Outdated
|
||
// style_general? pass array in instead of static? | ||
static void | ||
style_colors(struct nk_context* ctx, struct nk_color color_table[NK_COLOR_COUNT]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They're kind of mutually exclusive though. The Colors tab applies to all styles, so Colors->COLOR_TEXT is far more than Text->Color. There's no logical way to connect the latter to the former. The best (and I think most reasonable thing) I could do is maybe add a return value to style_rgb and not actually apply the color table unless one of the color drop downs in style_colors was clicked (even if they didn't change it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could rename the tab (and maybe the function) to be something like "Global Colors" or "Color Table"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that could be a reasonable compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Obviously the general use case is do global color styling first and then tweak individual colors as needed in the other tabs.
89e8997
to
58a46db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few extra comments
demo/style_configurator.c
Outdated
const char* alignments[] = | ||
{ | ||
"LEFT", | ||
"CENTERED", | ||
"RIGHT", | ||
"TOP LEFT", | ||
"TOP CENTERED", | ||
"TOP_RIGHT", | ||
"BOTTOM LEFT", | ||
"BOTTOM CENTERED", | ||
"BOTTOM RIGHT" | ||
}; | ||
|
||
#define TOP_LEFT NK_TEXT_ALIGN_TOP|NK_TEXT_ALIGN_LEFT | ||
#define TOP_CENTER NK_TEXT_ALIGN_TOP|NK_TEXT_ALIGN_CENTERED | ||
#define TOP_RIGHT NK_TEXT_ALIGN_TOP|NK_TEXT_ALIGN_RIGHT | ||
#define BOTTOM_LEFT NK_TEXT_ALIGN_BOTTOM|NK_TEXT_ALIGN_LEFT | ||
#define BOTTOM_CENTER NK_TEXT_ALIGN_BOTTOM|NK_TEXT_ALIGN_CENTERED | ||
#define BOTTOM_RIGHT NK_TEXT_ALIGN_BOTTOM|NK_TEXT_ALIGN_RIGHT | ||
|
||
int aligns[] = | ||
{ | ||
NK_TEXT_LEFT, | ||
NK_TEXT_CENTERED, | ||
NK_TEXT_RIGHT, | ||
TOP_LEFT, | ||
TOP_CENTER, | ||
TOP_RIGHT, | ||
BOTTOM_LEFT, | ||
BOTTOM_CENTER, | ||
BOTTOM_RIGHT | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is probably a better way to write this :)
struct {
const char *name;
int align;
} alignments = {
{ "LEFT", NK_TEXT_LEFT },
...
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth having to create a callback and use nk_combo_callback? It would be nice if there were something between nk_combo and nk_combo_callback, something that you just give an offset and stride to so nuklear can access the strings directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I finally looked into this some more and I have a version stashed but I really think it's worse. It's at least as long if not longer, more complicated, and has more disparate/dislocated parts.
demo/style_configurator.c
Outdated
|
||
// TODO better way? | ||
static int initialized = nk_false; | ||
static struct nk_color color_table[NK_COLOR_COUNT]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An idea could be to just let the user pass in the color table.
static int
style_configurator(struct nk_context *ctx, struct nk_color color_table[NK_COLOR_COUNT])
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
6d34a41
to
6bd90a9
Compare
Btw, just ping me when you feel the code is ready for another review. I'll then take a look and get this merged. |
There's not really anything else to do but I'm in no hurry to merge it. We might as well wait till after your nk_slice/API change so you'll have one less demo to worry about updating yourself. Plus it'll give me practice with the new changes. |
Awesome job - getting a few warnings and compile errors when I include the c file into my c++ sources (it works for the other demos):
|
|
6bd90a9
to
27f53d6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't know about such a project, which is quite great, thank you for suggesting it.
First advice: put style_configurator.c in common, with other files common to all backends.
I am on Linux and many constructs do not work with make flags that demos contain:
- I had to change to c99 instead of c89 to avoid many errors and warning
- Some specific constructs are still not allowed in c99 because of the -pedantic flag (which I prefer to keep)
I know demos don't have to follow Nuklear's ansi C compatibility, but that would be great to keep it still :)
When the PR is merged, I can provide fixes if needed.
Let me know if there's anything else but it's pretty much done. |
LGTM, tested on Linux (sdl/opengl3 backend). If fixes are needed for other demos, they can surely be done afterwards. Thanks again for this tool, I will certainly play with it soon (and probably request improvements ^^) |
@rswinkle sorry that makes more than a year haha There is a conflict on main due to other changes from other commits. Could you rebase from master and give the PR another round please? Promise this time I'll be around to checkout ;-) |
Now it only updates the colors after one of the dropdowns has been clicked, not every frame that the tree/tab is expanded
b8064c7
to
0f90945
Compare
I made this for myself it working on my own Nuklear projects and also to help me understand how the various styles work and interact.
I think it (and the individual functions) would be useful to others both for app developers and developing and debugging Nuklear itself. I could also see adding more configuration for values that aren't (currently) exposed in the style structures but figured I'd leave that for future work.
While I've been testing it manually/informally while I've written it, there was a lot of copy/pasting involved so I'm sure it's inevitable that a few bugs slipped through, which is why this is marked as a RFC. I definitely want people to try it on more backends (I tested on sdl_opengl3), but also just want general suggestions/critiques.