Skip to content

Conversation

@Demorome
Copy link
Contributor

@Demorome Demorome commented Jan 11, 2026

This is useful so that users have the most up-to-date information regarding the window.

For example, without this, it's possible for the Window class' SizeChangeCallback to not be called before the Draw function, which could cause a crash if the user's Draw function assumes that the swapchain's width/height are the same as the last reported values from the SizeChangeCallback / the values reported by the game's main Window (as I experienced, for a region-based blit).

@Demorome
Copy link
Contributor Author

Demorome commented Jan 11, 2026

It may be more prudent to somehow always call GatherSDLEvents + ProcessSystemEvents right after* the swapchain is acquired, instead, to prevent any micro-lag from getting in the way of accuracy.

Still, this seems to have stopped the crashes I was experiencing, for now.

@thatcosmonaut
Copy link
Contributor

I don't think it's a good idea to do this. Instead, I think we should just not provide this callback and the client should be expected to always use the size value returned by AcquireSwapchainTexture. This is because SDL has its own window size change filter that it uses to determine the size of the swapchain, so that value will always be up to date.

@thatcosmonaut
Copy link
Contributor

As some additional explanation, doing this would be bad mostly because it would cause input events to be gathered long before they are processed.

@Demorome
Copy link
Contributor Author

Demorome commented Jan 12, 2026

I agree with your proposed solution for window size change handling, but what about SDL_EVENT_WINDOW_DISPLAY_SCALE_CHANGED and SDL_EVENT_WINDOW_PIXEL_SIZE_CHANGED? (It doesn't seem like the former event is being handled currently, but perhaps it should be.) Having those scale values be outdated, even for a single frame, could cause erroneous calculations for UI scaling. Although, I suppose the damage could easily be caught and restrained by limiting ourselves to the swapchain texture's size, and this would be a rather rare issue.
2nd edit: NVM, HandleWindowPixelSizeChangeEvent is only used to update window size; Window.DisplayScale is never cached and is always queried. I see that caching would have no benefit in this case, since we'd always need to query it to double-check anyways.

Your second argument isn't as clear to me. The only issue I can imagine with gathering input events long before processing would be that internally, SDL performs optimizations by merging input events of the same type for the same device, so we don't need to process multiple of such events. I didn't see this being mentioned in the user-facing docs, but I suppose it is reasonable to assume as a backend detail.
EDIT: Oops, this was implicit with the implicit call to SDL_PumpEvents, so yeah you're right, gathering early is a bad idea for performance.

@Demorome
Copy link
Contributor Author

Demorome commented Jan 12, 2026

To address your points, I'll drop my current proposal for another.

I propose first keeping the callback, but internally calling it whenever we call AcquireSwapchainTexture and the size isn't matching with the passed Window's cached size. It would only require a check before internally dispatching the event to users to prevent a redundant dispatch. It seems more convenient, at little cost. I believe a user would have to implement such callback-redundancy-checks themselves, anyways, if they needed to check if re-building a render texture was necessary or not. Additionally, it would ensure that Window.Height/Width aren't footguns, assuming they weren't intended to be removed with your proposed solution.

When calling AcquireSwapchainTexture, we can also query for the display scale and pixel density changes after the swapchain texture is acquired, instead of solely relying on events, so that we have the most up-to-date info no matter what. Again, this would require a check to prevent redundant user event dispatch. I'm willing to part with this idea if querying is too expensive in this context, and just add a warning that the info may be out-of-date for a frame.
Due to the above comment's 2nd edit, this won't be needed.

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