Skip to content
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

Fix Displays querying on every access and add DisplaysChanged #5439

Merged
merged 7 commits into from
Oct 4, 2022

Conversation

Susko3
Copy link
Member

@Susko3 Susko3 commented Oct 3, 2022

peppy reported it on discord: Displays were queried in a hot path (every frame).

Adds DisplaysChanged event to IWindow so consumers can watch for display changes. For now, this doesn't check if the displays actually changed, and will be invoked every time the displays are queried.

`Displays` are used in a hot path in `UserInputManager.mouseOutsideAllDisplays`
so storing is much more performant than querying SDL every time.
To support `SequenceEqual` as expected, `Display` equality has to be changed.
This should also make `CurrentDisplayBindable` more accurate when
something other than the index changes.
@Susko3 Susko3 changed the title Fix Displays querying on every access Fix Displays querying on every access and add DisplaysChanged Oct 3, 2022
@peppy peppy self-requested a review October 4, 2022 04:01
Copy link
Member

@peppy peppy left a comment

Choose a reason for hiding this comment

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

I think this looks correct

Comment on lines +123 to +127
/// <summary>
/// Invoked when <see cref="Displays"/> has changed.
/// </summary>
[CanBeNull]
event Action<IEnumerable<Display>> DisplaysChanged;
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason a callback event is used instead of making the displays list into a bindable list instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had contemplated using a BindableList, but this was scrapped for multiple reasons:

  • using it as ItemSource in dropdowns would make sense, but this crashes as it's not on the update thread
  • updating all the elements in the list requires Clear()ing it first, and this would cause trouble with things that expect at least one available display
  • BindCollectionChanged / NotifyCollectionChangedEventArgs doesn't provide the actual list (existing consumer code expects a list)
  • the way displays can change means that rebuilding the entire list is almost always necessary, so an event callback is simpler and gets the job done

Copy link
Member

Choose a reason for hiding this comment

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

  • using it as ItemSource in dropdowns would make sense, but this crashes as it's not on the update thread

Dropdown should be responsible for scheduling updates of ItemSource, similar to how it already does with Current.

  • BindCollectionChanged / NotifyCollectionChangedEventArgs doesn't provide the actual list (existing consumer code expects a list)

A BindableList can be iterated as a list, while NotifyCollectionChangedEventArgs would only inform addition/removal of displays.

  • updating all the elements in the list requires Clear()ing it first, and this would cause trouble with things that expect at least one available display
  • the way displays can change means that rebuilding the entire list is almost always necessary, so an event callback is simpler and gets the job done

Fair enough, BindableList would be a nice addition to simplify listing of displays, but I guess it could be left off as it is until it's necessary.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants