Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Add tooltip to 3d floating buttons #7266

Merged
merged 2 commits into from
Dec 21, 2023
Merged

Conversation

2metres
Copy link
Contributor

@2metres 2metres commented Dec 20, 2023

User-Facing Changes
None

Description
Use MUI tooltip instead of native title element

Screen.Recording.2023-12-20.at.8.48.56.pm.mov

@defunctzombie
Copy link
Contributor

The reason we hadn't done this before is because the mui tooltip has very poor performance for re-rendering when we last profiled it and I am hesitant to add another use of it in a location that renders at 60fps. Have we fixed these performance concerns?

Copy link
Contributor

@defunctzombie defunctzombie left a comment

Choose a reason for hiding this comment

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

See comment about performance concerns. Convince me this is ok to add to a panel rendering at 60fps.

@2metres
Copy link
Contributor Author

2metres commented Dec 20, 2023

Do we have something documenting the how to test the performance benchmarks that we need to pass for this?

@2metres
Copy link
Contributor Author

2metres commented Dec 20, 2023

Throwing a tooltip around a button in the ui-benchmarking tool certainly doesn't bode well.

Screen Shot 2023-12-21 at 9 09 40 am

@jtbandes
Copy link
Member

One would hope that the RendererOverlay is not re-rendering at 60fps, during panning or playback. I am not saying I know for sure it doesn't – just that I would hope it doesn't 😅

@2metres
Copy link
Contributor Author

2metres commented Dec 21, 2023

One would hope that the RendererOverlay is not re-rendering at 60fps, during panning or playback. I am not saying I know for sure it doesn't – just that I would hope it doesn't 😅

From memory this should be true. But I also have no way of testing this

@2metres
Copy link
Contributor Author

2metres commented Dec 21, 2023

From what I can tell the RendererOverlay does not re-render at 60fps. I think this PR is safe to merge.

@defunctzombie defunctzombie dismissed their stale review December 21, 2023 14:52

Investigated the render overlay and the learning is that we do not think it renders at 60 fps

Copy link
Contributor

@defunctzombie defunctzombie left a comment

Choose a reason for hiding this comment

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

👍

Unrelated to the PR but it would be nice to find ways of using smaller component files for some of these things. It would make it more obvious which components use what state and selectors and what will cause them to re-render. I think we should be doing that across the codebase more.

@2metres 2metres merged commit f002c41 into main Dec 21, 2023
14 checks passed
@2metres 2metres deleted the 2metres/FG-5490-toggle-button-tooltip branch December 21, 2023 21:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants