Add sdl3_renderer demo V4 #852
Conversation
This comment was marked as off-topic.
This comment was marked as off-topic.
RobLoach
left a comment
RobLoach
left a comment
There was a problem hiding this comment.
Thanks again for the push one this.
sleeptightAnsiC
force-pushed
the
feat__sdl3_renderer_demo
branch
from
00c67fd to
abae018
Compare
sleeptightAnsiC
force-pushed
the
feat__sdl3_renderer_demo
branch
from
abae018 to
d36bc86
Compare
|
@sleeptightAnsiC |
|
🎉Congratulations! CLICK
Screenshot 3 |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Think it would be okay to merge, and fix the remaining in follow ups? Or should we wait? |
This comment was marked as outdated.
This comment was marked as outdated.
|
Looked pretty good to me. Wasn't able to run it since I'll need a CMake implementation, but we can fix any of the other small things up afterwards. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
High DPI support patch is done and ready. 🎉🥳 ✅ Key features:
I guess, with current Nuklear API there is essentially no better DPI handling possible, we adjust scale via high_dpi.patchdiff --git a/old_main.c b/main.c
index edfd5a9..402f6ec 100644
--- a/old_main.c
+++ b/main.c
@@ -183,6 +183,96 @@ nk_sdl_fail()
return SDL_APP_FAILURE;
}
+static void
+sdl_demo_set_debug_font(void* appstate)
+{
+ struct nk_sdl_app* app = (struct nk_sdl_app*)appstate;
+ struct nk_context* ctx = app->ctx;
+ nk_sdl_style_set_debug_font(ctx);
+
+ /* note that since debug font is extremely small (only 8x8 pixels),
+ * you may wish to change a few style options, here are few recommendations: */
+ ctx->style.button.rounding = 0.0f;
+ ctx->style.menu_button.rounding = 0.0f;
+ ctx->style.property.rounding = 0.0f;
+ ctx->style.property.border = 0.0f;
+ ctx->style.option.border = -1.0f;
+ ctx->style.checkbox.border = -1.0f;
+ ctx->style.property.dec_button.border = -2.0f;
+ ctx->style.property.inc_button.border = -2.0f;
+ ctx->style.tab.tab_minimize_button.border = -2.0f;
+ ctx->style.tab.tab_maximize_button.border = -2.0f;
+ ctx->style.tab.node_minimize_button.border = -2.0f;
+ ctx->style.tab.node_maximize_button.border = -2.0f;
+ ctx->style.checkbox.spacing = 5.0f;
+
+ /* it's better to disable anti-aliasing when using blocky fonts */
+ app->AA = NK_ANTI_ALIASING_OFF;
+}
+
+static void
+sdl_demo_set_font(void* appstate, float font_scale)
+{
+ struct nk_sdl_app* app = (struct nk_sdl_app*)appstate;
+ struct nk_context* ctx = app->ctx;
+ struct nk_sdl* sdl = (struct nk_sdl*)ctx->userdata.ptr;
+
+ struct nk_font_atlas* atlas;
+ struct nk_font_config config = nk_font_config(0);
+ struct nk_font* font;
+ /* set up the font atlas and add desired font; note that font sizes are
+ * multiplied by font_scale to produce better results at higher DPIs */
+ /* FIXME: This looks odd, but all existing renderer no calls nk_font_atlas_clear
+ * inside nk_sdl_font_stash_begin. This might cause a leak if called multiple times */
+ atlas = nk_sdl_font_stash_begin(ctx);
+ font = nk_font_atlas_add_default(atlas, 13 * font_scale, &config);
+ /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/DroidSans.ttf", 14 * font_scale, &config);*/
+ /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/Roboto-Regular.ttf", 16 * font_scale, &config);*/
+ /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/kenvector_future_thin.ttf", 13 * font_scale, &config);*/
+ /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/ProggyClean.ttf", 12 * font_scale, &config);*/
+ /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/ProggyTiny.ttf", 10 * font_scale, &config);*/
+ /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/Cousine-Regular.ttf", 13 * font_scale, &config);*/
+ nk_sdl_font_stash_end(ctx);
+
+ /* this hack makes the font appear to be scaled down to the desired
+ * size and is only necessary when font_scale > 1 */
+ font->handle.height /= font_scale;
+ /*nk_style_load_all_cursors(ctx, atlas->cursors);*/
+ nk_style_set_font(ctx, &font->handle);
+
+ app->AA = NK_ANTI_ALIASING_ON;
+}
+
+static void
+sdl_demo_on_scale_changed(void* appstate, float scale)
+{
+ struct nk_sdl_app* app = (struct nk_sdl_app*)appstate;
+ struct nk_context* ctx = app->ctx;
+ struct nk_sdl* sdl;
+
+ NK_ASSERT(scale > 0);
+ SDL_SetRenderScale(app->renderer, scale, scale);
+ /* If you don't want to use advanced Nuklear font baking API
+ * you can use simple ASCII debug font provided by SDL
+ * just change the `#if 0` above to `#if 1` */
+#if 0
+ {
+ sdl = (struct nk_sdl*)ctx->userdata.ptr;
+ NK_ASSERT(sdl);
+ /* According to SDL3 documentation, debug font glyphs are always
+ * 8 pixels high, so no scaling is needed when the display DPI changes */
+ if (!sdl->debug_font)
+ sdl_demo_set_debug_font(appstate);
+ }
+#else
+ {
+ NK_UNUSED(sdl);
+ /*scale*2 will give even better quality for ProggyClean*/
+ sdl_demo_set_font(appstate, scale);
+ }
+#endif
+}
+
SDL_AppResult
SDL_AppInit(void** appstate, int argc, char* argv[])
{
@@ -201,7 +291,7 @@ SDL_AppInit(void** appstate, int argc, char* argv[])
return nk_sdl_fail();
}
- if (!SDL_CreateWindowAndRenderer("Nuklear: SDL3 Renderer", WINDOW_WIDTH, WINDOW_HEIGHT, SDL_WINDOW_RESIZABLE, &app->window, &app->renderer)) {
+ if (!SDL_CreateWindowAndRenderer("Nuklear: SDL3 Renderer", WINDOW_WIDTH, WINDOW_HEIGHT, SDL_WINDOW_RESIZABLE | SDL_WINDOW_HIGH_PIXEL_DENSITY, &app->window, &app->renderer)) {
SDL_free(app);
return nk_sdl_fail();
}
@@ -216,77 +306,10 @@ SDL_AppInit(void** appstate, int argc, char* argv[])
app->bg.b = 0.24f;
app->bg.a = 1.0f;
-
- font_scale = 1;
- {
- int render_w, render_h;
- int window_w, window_h;
- float scale_x, scale_y;
- SDL_GetCurrentRenderOutputSize(app->renderer, &render_w, &render_h);
- SDL_GetWindowSize(app->window, &window_w, &window_h);
- scale_x = (float)(render_w) / (float)(window_w);
- scale_y = (float)(render_h) / (float)(window_h);
- SDL_SetRenderScale(app->renderer, scale_x, scale_y);
- font_scale = scale_y;
- }
-
ctx = nk_sdl_init(app->window, app->renderer, nk_sdl_allocator());
app->ctx = ctx;
-#if 0
- {
- /* If you don't want to use advanced Nuklear font baking API
- * you can use simple ASCII debug font provided by SDL
- * just change the `#if 0` above to `#if 1` */
- nk_sdl_style_set_debug_font(ctx);
- NK_UNUSED(font_scale);
-
- /* note that since debug font is extremely small (only 8x8 pixels),
- * you may wish to change a few style options, here are few recommendations: */
- ctx->style.button.rounding = 0.0f;
- ctx->style.menu_button.rounding = 0.0f;
- ctx->style.property.rounding = 0.0f;
- ctx->style.property.border = 0.0f;
- ctx->style.option.border = -1.0f;
- ctx->style.checkbox.border = -1.0f;
- ctx->style.property.dec_button.border = -2.0f;
- ctx->style.property.inc_button.border = -2.0f;
- ctx->style.tab.tab_minimize_button.border = -2.0f;
- ctx->style.tab.tab_maximize_button.border = -2.0f;
- ctx->style.tab.node_minimize_button.border = -2.0f;
- ctx->style.tab.node_maximize_button.border = -2.0f;
- ctx->style.checkbox.spacing = 5.0f;
-
- /* it's better to disable anti-aliasing when using blocky fonts */
- app->AA = NK_ANTI_ALIASING_OFF;
- }
-#else
- {
- struct nk_font_atlas *atlas;
- struct nk_font_config config = nk_font_config(0);
- struct nk_font *font;
-
- /* set up the font atlas and add desired font; note that font sizes are
- * multiplied by font_scale to produce better results at higher DPIs */
- atlas = nk_sdl_font_stash_begin(ctx);
- font = nk_font_atlas_add_default(atlas, 13 * font_scale, &config);
- /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/DroidSans.ttf", 14 * font_scale, &config);*/
- /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/Roboto-Regular.ttf", 16 * font_scale, &config);*/
- /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/kenvector_future_thin.ttf", 13 * font_scale, &config);*/
- /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/ProggyClean.ttf", 12 * font_scale, &config);*/
- /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/ProggyTiny.ttf", 10 * font_scale, &config);*/
- /*font = nk_font_atlas_add_from_file(atlas, "../../../extra_font/Cousine-Regular.ttf", 13 * font_scale, &config);*/
- nk_sdl_font_stash_end(ctx);
-
- /* this hack makes the font appear to be scaled down to the desired
- * size and is only necessary when font_scale > 1 */
- font->handle.height /= font_scale;
- /*nk_style_load_all_cursors(ctx, atlas->cursors);*/
- nk_style_set_font(ctx, &font->handle);
-
- app->AA = NK_ANTI_ALIASING_ON;
- }
-#endif
+ sdl_demo_on_scale_changed(app, SDL_GetWindowDisplayScale(app->window));
return SDL_APP_CONTINUE;
}
@@ -297,6 +320,11 @@ SDL_AppEvent(void *appstate, SDL_Event* event)
struct nk_sdl_app* app = (struct nk_sdl_app*)appstate;
switch (event->type) {
+ case SDL_EVENT_WINDOW_DISPLAY_SCALE_CHANGED:
+ sdl_demo_on_scale_changed(app, SDL_GetWindowDisplayScale(app->window));
+ /* In this version, DPI is managed by this demo itself,
+ * so we avoid forwarding this event to Nuklear to prevent potential double handling */
+ return SDL_APP_CONTINUE;
case SDL_EVENT_QUIT:
return SDL_APP_SUCCESS;
} |
|
I spent a little time today trying to figure out why properties weren't triggering the edit and this is the fix I came up with (not sure if setting the name really matters but I was trying to match code in nk_edit_buffer(): diff --git a/src/nuklear_property.c b/src/nuklear_property.c
index 68b47ba..a234628 100644
--- a/src/nuklear_property.c
+++ b/src/nuklear_property.c
@@ -405,6 +405,8 @@ nk_property(struct nk_context *ctx, const char *name, struct nk_property_variant
win->property.name = hash;
win->property.select_start = *select_begin;
win->property.select_end = *select_end;
+ win->edit.active = nk_true;
+ win->edit.name = hash;
if (*state == NK_PROPERTY_DRAG) {
ctx->input.mouse.grab = nk_true;
ctx->input.mouse.grabbed = nk_true;
@@ -420,6 +422,7 @@ nk_property(struct nk_context *ctx, const char *name, struct nk_property_variant
win->property.select_start = 0;
win->property.select_end = 0;
win->property.active = 0;
+ win->edit.active = nk_false;
}
}
NK_API void
Seems to be working correctly. I also added a text field to the small demo window to test the differences and discovered something else. Text input stays active for a text field even if you click on (for instance) a color drop down, even if you actually click around in the color box or drag the nested channel properties. But it goes inactive if it was the property that was being edited. Just another small thing to dig into in the combobox/dropdown code. |
|
[replying to multiple messages at the same time] @rswinkle So this is more of a bug inside of Nuklear, right? If I understand your comment, the widgets are not setting their @PavelSharp I think, I fully understand your patch, but I find the font rebaking to be a bit of an overkill. It increases the complexity of @RobLoach I can bring back the cmake file. I didn't like the old one from #779, because it was silently downloading SDL sources instead of reusing the system-wide copy of it. I guess, since SDL3 is now primary cmake-based, it makes sense to have it here. Anyway, thank you guys for your input and patches. I hope I will deal with this soon. Sorry, for the slowdown on my part. I am a bit exhausted with debugging Nuklear and grepping its old commit messages |
This comment was marked as outdated.
This comment was marked as outdated.
I don't see how adding a callback would solve any issue here. It could be a nice addition, but the callback needs to be call'ed by something. I'm not gonna introduce any callback as part of this PR and I won't change my mind about it. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
I have to agree with @sleeptightAnsiC on this.
I would not call either my approach or the current approach fragile. As sleeptightAnsiC said, all approaches at their optimal/minimal state amount to the same thing, and in fact the callback method probably requires the most in actual lines changed. The logic is not/would not be in the callback. It is a matter of Nuklear internal state regardless. Knowing where/when to insert the callback is the same knowledge you would need for any other method.
Again, these are Nuklear bugs that need to be fixed in any case and the code changes are very small. It's a matter of discovery/precision.
The request and release does not happen at the widget level, that's part of the issue. It has to be aggregated over all the windows/widgets over the whole frame being rendered, and at that point, you know if either Start or Stop should be called based on whether it changed from the previous. Whether you use my method, adding a couple of members to try to simplify the test by collating as we go, or try to get the same result using a function like nk_is_any_active() combining hopefully all necessary checks (or both), the solution is the same. Ideally all widgets using text input should use the same method directly or (since that's obviously not the case) they should pass that state along to the text_edit state as the "master" state.
There is no breaking change regardless. Nuklear is not used as a dynamic library that will be updated separately from the application(s) that use it. It is meant to be compiled statically. And the changes are not what I would consider "breaking": Either a couple added field to an internal structure, an extra utility function meant almost exclusively for backend implementers not average users, or both. No change to the existing API, and nothing removed that could break someone's code.
I still don't really understand the point of this function but all the logic above applies here too afaict. The difficulty is in getting the information in the first place.
None of them are hacky workarounds. As I said all would require roughly the same logic/changes, the callback would just require a bit more code for the macro stuff at the top. My first attempt at doing this was a macro based callback system a la NK_IS_WORD_BOUNDARY() which I worked on a while back. It was two optional macros NK_ON_EDIT_FOCUS()/NK_ON_EDIT_UNFOCUS(). I ended up deleting it and replacing it twice, ending up on my current version because I realized everything that I discussed above. Not only do we not want or need to call Start/StopText() within Nuklear (even indirectly), but everything boils down to correctly and efficiently tracking the boolean state so the backend/user can do it themselves if they want to. What should be documented is all the knowledge about how that state is tracked which is what someone implementing a new text input widget not based on the nk_do_edit() family needs to know. As long as they remember the few lines to set the state appropriately then everything would just work.
See above. It's not that many changes and you would have to touch the same pieces of the code for the callback method. |
This comment has been minimized.
This comment has been minimized.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
Other "editable" widgets are updating the its window 'edit.active' state, but not nk_property. SDL3 demo needs this in order to hook SDL_SetTextInput. This patch was initially proposed by https://github.com/rswinkle The only thing I changed in it, was a removal of 'edit.name' update, nk_property is not using the name hash, so there is no reason to set it. Origin: Immediate-Mode-UI#852 (comment) Co-authored-by: Robert Winkler <[email protected]>
This is a highly updated demo for libSDL3. It fixes a lot of long occuring issues and brings some improvements specific to SDL3 API. The origin and development of this demo is quite long. The exact track of development is something like this: `Nuklear/demo/sdl_renderer` -> Immediate-Mode-UI#772 -> Immediate-Mode-UI#779 + Immediate-Mode-UI#825 -> _this_ commit... You may wish to read the linked PRs in order to get the full context. Commit message is too small to describe everything. I'm sorry...
sleeptightAnsiC
force-pushed
the
feat__sdl3_renderer_demo
branch
from
d36bc86 to
ec5e8d7
Compare
|
The latest force-push brings following changes:
I hope, this will bring a compromise to how TextInput should be tracked and updated, at least for now. PR no longer changes nk_* API. I made it so it will be easy to tweak TextInputActive and solve TextInputArea in the future. I may have some disagreement with Pavel, but I don't want to sabotage his work from #857 This is the most "friendly" solution I can see at the moment, and I hope that everyone will understand. Please, do not drag this PR for any longer with features that can be solved later. @PavelSharp @rswinkle Maybe, let's hide our latest off-topic comments, and/or move their content to #857 ? |
This comment was marked as resolved.
This comment was marked as resolved.
@RobLoach Hey, if you want to test this PR, the old file from #779 works just fine after few changes. Here is an updated version [attachment below]. Just make sure to drop it into the same directory as this demo. I'm not sure why it broke the last time I tested it. This time it "just" worked. 😳 I guess I can add it back as a separate commit (?) I'm not really a Cmake specialist, so I can't tell how good/portable it is. |
|
Thanks so much for all of the push on this, @sleeptightAnsiC , @PavelSharp , @rswinkle , @zoogies . So happy to have this finally in. |
|
Woo! Thanks, all. Glad to see this merged! Shoutout to @sleeptightAnsiC for taking the lead on this |
|
It's over... I'm released from my burden. Thanks for all testing and feedback. This was way bigger and harder to deal with, than I ever expected it to be, hah. Hopefully, all bugs found along the way will be soon followed by another fixes 😃 |
6 participants
This is yet another PR that implements SDL_renderer demo for libSDL3. I consider this to be a Version 4, because there are 3 other PRs that attempted to do this. This one fixes a lot of long occuring issues and brings some improvements specific to SDL3 API. I've put a lot of time and serious care into this, and I hope it will be useful for someone out there.
PR is split into two logical commits.
EDIT: I collapsed the notes because they were scary to look at, and probably outdated at this point. I doubt anyone fully reads them anyway.
notes [CLICK ME]
NOTEs:
Nuklear/demo/sdl_renderer-> [DRAFT] SDL3_renderer and SDL_gpu support #772 -> Add SDL3 #779 + Add sdl3 renderer demo #825 -> this PR...nk_sdl_handle_grab(), despite that I considered it, due to reasoning from Add SDL3 #779 (comment)userdatais handled, despite some concerns, per https://github.com/Immediate-Mode-UI/Nuklear/pull/779/files#r2487912696FIXMEin this PR is an issue within Nuklear, that I couldn't solve from the demo side. Each one of those should be already reported and linked to Github. If you see any FIXME without explanation or link, let me know.nk_sdl_allocator()to get Nuklear-compatible allocator provided by SDL. This is consistent with how Nuklear API works, but not exactly with the rest of demos.nk_sdl_style_set_debug_font()which let's you use primitive SDL debug font and allows to remove the dependency on font baking entirely (not used by default)TODOs (out of the scope of this PR):
nk_sdl_style_set_debug_font(), most of those are related to the Examples being hardcoded to specific font size, but there is also one severe bug: the background of color picker always appears black. Not sure if this is my fault or something in Nuklear.