Skip to content

refactor: sdl3::events - Modernize/Update SDL3 Events#258

Open
marstaik wants to merge 2 commits into
vhspace:masterfrom
marstaik:events-update
Open

refactor: sdl3::events - Modernize/Update SDL3 Events#258
marstaik wants to merge 2 commits into
vhspace:masterfrom
marstaik:events-update

Conversation

@marstaik

Copy link
Copy Markdown

Hi, while using migrating from winit to SDL3, I noticed that some of the events strangely did not have all of the correct fields and names (In my particular case, I have multiple windows and many of the events were missing window_id).

Seeing as some of these events haven't been updates in years from their source, I went ahead and modernized the events a bit. Thankfully, as you can see in the examples, their replacement/updated usage is pretty easy to follow.

Key changes:

  • Group related raw SDL events under category enums (Window, Keyboard, Mouse, Joystick, Controller, etc.).
  • Preserve ability to convert to/from raw SDL_Event (where feasible).
  • Keep the name Event so external code continues to reference crate::event::Event.
  • Maintain custom/user event registration and pushing.
  • Provide helper methods analogous to the old API (get_timestamp, get_window_id, is_* classifiers).

@marstaik marstaik changed the title Modernize SDL3 Events sdl3::events Refactor - Modernize/Update SDL3 Events Nov 14, 2025
@revmischa

Copy link
Copy Markdown
Member

Thank you! Happy to look/merge when CI is passing 😄

@marstaik

Copy link
Copy Markdown
Author

Yep, working on it, I missed some of them because of the feature flags

@marstaik marstaik changed the title sdl3::events Refactor - Modernize/Update SDL3 Events refactor: sdl3::events - Modernize/Update SDL3 Events Nov 15, 2025
@marstaik

Copy link
Copy Markdown
Author

I am considering adding a lifetime to events so that we can avoid cloning into Strings. If we add a lifetime bound to the event pump, since SDL handles backing memory we should be gracefully be able to just have CStr, and let the user decide when to allocate. I think its a better solution than just copying payloads.

Since a vast majority of the events don't have string references, the inner types should be trivially copyable anyways if you really need a copy.

This would facilitate better to_ll/from_ll behavior, but I'm wondering if we really need to_ll still...

@revmischa

Copy link
Copy Markdown
Member

Okay, please let me know when this PR is ready to merge. Do you want to add lifetimes here or later?

@revmischa

Copy link
Copy Markdown
Member

The direction here is right: grouping the flat SDL variants into structs with state enums mirrors how SDL3 itself models events, and the added accessors (is_*/as_*, get_timestamp, get_window_id) are nice.

The problem is it's drifted a long way from master. The branch predates several event changes that have since landed (WindowEvent::Occluded, the MouseWheel integer deltas, window_id on touch/finger events), and because it rewrites the whole file git shows it as an almost total conflict. As-is it would regress those: WindowEventKind has no Occluded variant, and MouseEvent::Wheel drops the integer x/y.

Could you rebase onto current master and fold those back in? Given the size and that it's a breaking change, it'd also be far easier to review (and to land as a major-version bump) if the core enum restructure were split from the example/test migration. Keen to get this in once it's caught up.

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.

2 participants