Skip to content

removed global object in SDL2 demo, more C++-friendly API #799

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

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,

if (ctx->input.mouse.grab) {
SDL_SetRelativeMouseMode(SDL_TRUE);
} else if (ctx->input.mouse.ungrab) {
/* 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