Exporting and loading colors adn styles RFC#914
Exporting and loading colors adn styles RFC#914rswinkle wants to merge 4 commits intoImmediate-Mode-UI:masterfrom
Conversation
...So you can change things like padding and spacing without widgets jumping around on you. Particularly useful when dragging properties.
|
Forgot to tag you guys. Let me know your ideas. |
|
Cool idea. Not sure what I did, but the window started to slow down, until I force closed it. Likely because I clicked on the button a bunch of times haha. Could likely use How do we end up using the output? Looks like it's a big JSON? |
| NK_API const struct nk_color* | ||
| nk_get_default_color_table(void) | ||
| { | ||
| return nk_default_color_style; | ||
| } | ||
| NK_API const char** | ||
| nk_get_color_names(void) | ||
| { | ||
| return nk_color_names; | ||
| } |
There was a problem hiding this comment.
(haven't read everything yet, neither run the code)
I don't mind the idea in general, because it will be a part of demo/common/style_configurator.c, but I would ask you to avoid exporting any internal global arrays at all cost.
All demos already reference these directly, and it's possible to do the same in style_configurator. Hopefully, we'll fix it someday, but for now it will leave us some room for improvement.
It's impossible to use these getters without reading the source code anyway. Caller needs to know what this array is, how to access the items correctly, needs to know the size is tied to NK_COLOR_COUNT, etc... Those are all internal details never meant for public API.
src/nuklear_style.c is a big pile of copy-pasted code with bunch of hacks around it, and I feel we should consider refactoring it someday. The fact it uses global arrays for abstraction is already "questionable". Knowing we can safely remove globals in the future would be beneficial. Having a public getter will obviously make it hard to deal with. That's really my point here.
Reading your PR comment, I've got an impression you already recognize the issue, so I hope my take is reasonable?
I though about cleaning up this mess few times, but it's very low prio on my list. Several widgets don't even resolve the colors correctly (#856 (comment)) and many styling fields are just unused dead code, and the types are all over the place (#891), and god knows what else is hiding there.
In the future, I though maybe we can abstract fields from styling structs (and their types and default values) into something like XMACRO? I believe, I've seen people talking about serializing those fields, and it's a general problem that I image everyone wants to solve on it's own, so having XMACROs that would let you quickly write a configurator/parser/loader/exporter feels like a good idea? That is, ofc, not related to this PR, and I don't expect anyone implementing this any time soon, but maybe you have some similar ideas in mind?
There was a problem hiding this comment.
I don't mind the idea in general, because it will be a part of
demo/common/style_configurator.c, but I would ask you to avoid exporting any internal global arrays at all cost.All demos already reference these directly, and it's possible to do the same in style_configurator. Hopefully, we'll fix it someday, but for now it will leave us some room for improvement.
It's impossible to use these getters without reading the source code anyway. Caller needs to know what this array is, how to access the items correctly, needs to know the size is tied to
NK_COLOR_COUNT, etc... Those are all internal details never meant for public API.
I disagree that having getters here is a bad thing, and I don't understand your reluctance, especially considering one of the things you've been working on for the last few months, namely, using Nuklear un-amalgamated. Though testing to make sure changes are made to the source files and not only the header is good, the idea that anyone would want to actually use Nuklear the source files directly instead of the amalgamated header is insane to me. But my point is that doing so would mean the user loses access to all these static globals and would need getters.
On the other hand, the far more likely use case, of using the amalgamated header but still in its own TU would have the same problem. You cannot do what I've done in sdl_img or style_configurator without having access to the color table, and whether you do it all in one TU or not, we should enable it as it is a fundamental part of Nuklear styling, for better or worse.
src/nuklear_style.cis a big pile of copy-pasted code with bunch of hacks around it, and I feel we should consider refactoring it someday. The fact it uses global arrays for abstraction is already "questionable". Knowing we can safely remove globals in the future would be beneficial. Having a public getter will obviously make it hard to deal with. That's really my point here.Reading your PR comment, I've got an impression you already recognize the issue, so I hope my take is reasonable?
nk_style and sub structures could definitely do with some changes, starting with the int -> nk_bool issues but the global array is not an issue imo. One way or another the user should be able to set and access the global color settings directly (without messing with other styling settings...technically we only have half of this since setting them resets all the styling as I mentioned before...another thing that should probably change, adding the ability to set global colors without touching other styling at the very least. Other than some kind of global, array or struct (basically the same if it's just colors), what would you suggest? Even if you had a getter that had the user pass in an array/struct for Nuklear to use, either you have an array/struct you're drawing from anyway or you're having to manually dig colors from the style structures which is ugly and misrepresents the relationship (it's 1 <-> many from global colors to style colors). And the "setter" would be the same ugly, just as it is today because of the 1-> many relationship, regardless of the source of the values.
In the future, I though maybe we can abstract fields from styling structs (and their types and default values) into something like XMACRO? I believe, I've seen people talking about serializing those fields, and it's a general problem that I image everyone wants to solve on it's own, so having XMACROs that would let you quickly write a configurator/parser/loader/exporter feels like a good idea? That is, ofc, not related to this PR, and I don't expect anyone implementing this any time soon, but maybe you have some similar ideas in mind?
I'm not sure what you mean here. Can you describe what this would look like in practice and how it would facilitate doing styling both as colors and the rest, separately and together? Also true serializing is problematic with the pointer members involved.
There was a problem hiding this comment.
I would ask you to avoid exporting any internal global arrays at all cost.
we should enable it as it is a fundamental part of Nuklear styling, for better or worse.
That wasn't the purpose of this PR, and it can be implemented without it, just like style_configurator does it already. There is really no need to change this right now. The fact demos are accessing those arrays is a hack on its own, but having public getter wouldn't make it less of a hack. Just getting a pointer is not enough without depending on implementation details. I really think this is a bad idea. Most of those examples won't work without single TU anyway.
one of the things you've been working on for the last few months [...] the idea that anyone would want to actually use Nuklear the source files directly [...] is insane to me.
Not sure if that sentence was phrased this way by accident. I really hope it was.
There was a problem hiding this comment.
That wasn't the purpose of this PR, and it can be implemented without it, just like style_configurator does it already. There is really no need to change this right now. The fact demos are accessing those arrays is a hack on its own, but having public getter wouldn't make it less of a hack. Just getting a pointer is not enough without depending on implementation details. I really think this is a bad idea. Most of those examples won't work without single TU anyway.
Sure, and I was planning to remove it as this is very much a DRAFT/RFC, but that doesn't mean it can't or shouldn't be discussed here. Discussions like this are half the reason for Drafts/RFC's. And I'm talking about Nuklear use in general, not just these demos/examples, though that single TU requirement is a bad thing imo, because people should be able to easily drop Overview, Configurator etc., into their project with no issues, regardless of how its structured.
Also, this is C and we are C programmers working on an open source project. Not only is there no such a thing as private technically, but it is almost guaranteed that people will be looking at the source code, and a fair number will be modifying it. The closest we get in C is things clearly labeled as "deep dark implementation details, do not touch unless you really know what you're doing" and possibly naming things with __ prefixes or something. I think I've seen macros for obfuscation once or twice. In any case, the color table definitely doesn't qualify as "private."
one of the things you've been working on for the last few months [...] the idea that anyone would want to actually use Nuklear the source files directly [...] is insane to me.
Not sure if that sentence was phrased this way by accident. I really hope it was.
That was definitely badly written with a critical typo. To clarify, I see 0 point to using Nuklear's component files individually rather than the combined header. I can't think of a use case. At most, I could maybe see modifying src/nuklear.h and paq.sh to not use certain parts, but still using the smaller combined header that results.
However, we did need a way to enforce/test that all changes were made to the source files so they and the output of paq.sh were never out of sync. I think that could have been done with a simple diff (paq output to a temporary file to compare to) but possibly there are other factors I'm missing. I do know something from the work that's been done fixed the math function warnings I was getting for years so I do appreciate it.
| } | ||
|
|
||
| static void | ||
| read_int(int* i, FILE* in) |
There was a problem hiding this comment.
On my machine, I'm getting bunch of warnings related to this function. Here's full output: https://gist.github.com/sleeptightAnsiC/8a5ab42c001c24def983823a5a7c285e
There was a problem hiding this comment.
That's expected. I just wanted to get something compiling and working and didn't care about the warnings as they can always be fixed later. The enums are annoying though as it means we can't generalize and have to have separate code for every type to avoid those warnings.
Agree, it's a bit strange, just braces and raw values. Not really human readable, and serialized file will break when adding any new fields (am I right?) Do we have any alternatives we can consider here?
I've tried doing:
and it segfaulted ;/ EDIT: here is full callstack: https://gist.github.com/sleeptightAnsiC/c3dda841cdfc8f99cfbdc5f48dc5bb63 |
|
|
||
|
|
||
|
|
||
| #if 0 |
There was a problem hiding this comment.
what's going on here?
There was a problem hiding this comment.
It was just a reference when I first started, to see all the exploded members. I can remove it now.
I'll have to look into both of those. And file selection would be the most standard but now that I think about it before I submit my file browser, I don't think it has a "make new file (or folder)" option as I've only ever used it for pure selection. Sigh.
Ok clearly I didn't do a good enough job explaining and the video didn't clarify as much as I hoped. So the format (other than the labeled colors selection) is meant to be C/C++ declarations so you can just copy paste it into code, add a name call nk_style_from_table and/or memcpy it into nk_style (both if you want full flexibility). Since I'm obviously doing live changes, my load functions are just dumb parsers for that same format. This is why I'm doing file redirection for both output and input, because it's a lot of text and it's pain to copy/past after the fact. I suspect both your problems are from mismatching your input with your output. Obviously if you try to load something that is not what you provided it will hang/fail. If you want to do everything, only use the export and load buttons at bottom of the window for both colors and styles. I don't (yet) have export/load buttons for styles alone, like I do for global colors in the global colors section. If you do want to just test colors, you can use those buttons but don't use the named format. I don't have an import for that format as it's meant for specific config files like Lua. I would do designated initializers so we could have the best of both worlds (valid C and more readable/understandable) but Nuklear is C89. |
As mentioned in my response to Rob, it's supposed to be C syntax, just not prettified. That can be added later but it's really not meant to be user readable/editable. This is C89 unfortunately, we don't get designated initializers to help with that.
I'm not sure what you did either but I assume it's wayland's fault because it worked fine for me ;-) 
I would recommend doing plain file redirection like I do in my original video so you can look at the output file and verify it's correct if you're having problems. |
3 participants
My very rough draft/exploration of your idea
Doing just the global color table is easy, though there are still lots of options (include alpha or not, have labels or not (for nicer readable startup configuration files). Right now I switched the GUI to use integer properties so I wouldn't have to convert back and forth, but am still not including an alpha property to modify though I do print it out on export. And I have 2 export options.
The bigger problem is the nk_style struct. It's huge and complicated and full of items that don't really make sense to export, like cursors and callbacks and such, and union types (nk_style_item) where I only support color. The output is also ugly and definitely not user friendly. I could make it prettier but it's really not designed for user editing, whether compile or run time. All the sub structures have their own functions so users could make their own custom load/save functions getting only the parts they need.
Lastly the interaction between the color table and the style is also problematic. The color table is not part of the style structure and using it overwrites the style structure (and not just colors, but padding and spacing and all the other things too which is not expected). So, if all you want to modify is colors and you're happy with coarse granularity, the color table is sufficient, but if you want to change any other properties, or change the text color in certain widgets to be different than others, you have to do your global color changes first, then your individual style changes, then export colors and styles. Then to use them you load colors and styles (in that order) so you get the state you saved back.
Lastly there's the issue of GUI/ergonomics for exporting/loading. Right now I'm just using stdout/stdin and redirecting files for testing but we'd really want a file selection dialog to make this more user friendly. I always did want to get my file browser into the repo as a more easy to use solution than our existing demo...
Screencast.from.2026-03-09.21-20-27.mp4
I honestly don't know why the text gets all screwed up in the video when it turns red, it looks fine for me. I'm just using the built in screenshot tool not OBS or anything so I can only blame that. Even if you can't read the red text it should be obvious that it correctly reloads the style structure with the modified white button text and increased. horizontal window spacing.