Skip to content

Conversation

@Xeverous
Copy link
Contributor

@Xeverous Xeverous commented Apr 1, 2025

I have modified SDL2 demo to 1) remove the global object 2) make interface more open and C++-friendly (most notably nk_context and SDL_Window are no longer a part of nk_sdl). Ideally I would remove the nk_sdl entirely or move all "official" nk_ types out of it so that one can use them independently. My C++ code builds separate classes for nk_context, nk_font_atlas etc so if any demo/example code assumes certain layout of them (or stores them in a global object) it causes API incompatibility problems. Best if the C code just takes pointers and allows the callee to manage lifetime.

This is not a final state of the code I envision, it's more of a proof of concept that such API is possible. I want Nuklear examples to follow it's library design which means:

  • no global or hidden state
  • never assuming ownership of provided data
  • not forcing the caller to any particular composition of data (everything as much separate as possible, passed by pointers)

{
/* setup global state */
struct nk_sdl_device *dev = &sdl.ogl;
struct nk_sdl_device *dev = &sdl->ogl;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if any of ctx, sdl, win and similar pointers are nullptr in the NK_API type interfaces?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

UB obviously. This patch isn't a real PR. It's just a proof of concept and to ask library maintainers whether they like this direction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While it is a big change in how the demo is set up, I believe the benefits out of thread-safety does outweigh the cons of backward compatibility breaks. We could add some NK_ASSERT()s to ensure there arn't any nullptrs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes yes I will improve the patch by adding some comments (to better explain the API), do similar changes in other SDL examples and add missing assertions,

/* better support for older SDL by setting mode first; causes an extra mouse motion event */
SDL_SetRelativeMouseMode(SDL_FALSE);
SDL_WarpMouseInWindow(sdl.win, (int)ctx->input.mouse.prev.x, (int)ctx->input.mouse.prev.y);
SDL_WarpMouseInWindow(win, (int)ctx->input.mouse.prev.x, (int)ctx->input.mouse.prev.y);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As window is now possible to be recreated and is independent from the Nukelar context could it happen that prev mouse input was on prev big window that is outside of new small window? Could Nuklear context aware of old window cause problems if new window is passed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I don't know how the interaction here works exactly but I guess some additional function could be added (for the caller) to adjust the state if a new window object is provided.

@RobLoach RobLoach mentioned this pull request May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants