-
Notifications
You must be signed in to change notification settings - Fork 615
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
base: master
Are you sure you want to change the base?
removed global object in SDL2 demo, more C++-friendly API #799
Conversation
demo/sdl_opengl2/nuklear_sdl_gl2.h
Outdated
{ | ||
/* setup global state */ | ||
struct nk_sdl_device *dev = &sdl.ogl; | ||
struct nk_sdl_device *dev = &sdl->ogl; |
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.
What happens if any of ctx, sdl, win and similar pointers are nullptr in the NK_API type interfaces?
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.
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.
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.
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.
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.
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); |
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.
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?
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'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.
(more work required: edit more examples, deduplicate code and improve API)
I have modified SDL2 demo to 1) remove the global object 2) make interface more open and C++-friendly (most notably
nk_context
andSDL_Window
are no longer a part ofnk_sdl
). Ideally I would remove thenk_sdl
entirely or move all "official"nk_
types out of it so that one can use them independently. My C++ code builds separate classes fornk_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: