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 infinite recursion when putting a container inside a viewer tab #3534

Merged
merged 4 commits into from Sep 29, 2023

Conversation

emilk
Copy link
Member

@emilk emilk commented Sep 29, 2023

What

In egui_tiles, not all tiles are panes with space views - some are containers, with more tiles in them.

The existing code on main tried to handle the latter by calling the trait default implementation of a function:

    fn tab_ui(
        &mut self,
        tiles: &egui_tiles::Tiles<SpaceViewId>,
        ui: &mut egui::Ui,
        id: egui::Id,
        tile_id: egui_tiles::TileId,
        active: bool,
        is_being_dragged: bool,
    ) -> egui::Response {
        let Some(tab_widget) = TabWidget::new(self, ui, tiles, tile_id, active, 1.0) else {
            return egui_tiles::Behavior::<SpaceViewId>::tab_ui(
                self,
                tiles,
                ui,
                id,
                tile_id,
                active,
                is_being_dragged,
            );
        };}

…the problem is that this is a recursive call, leading to infinite recursion (and the compiler isn't catching it for whatever reason).

The new code in this PR explicitly handles groups, giving them an icon and a name:

tab-container-fix

Checklist

@emilk emilk added 🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start include in changelog labels Sep 29, 2023
@emilk emilk marked this pull request as ready for review September 29, 2023 08:34
@Wumpf Wumpf self-requested a review September 29, 2023 08:38
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Nicely done! Sadly the selection behavior doesn't work out for this yet:

Screen.Recording.2023-09-29.at.11.02.17.mov

and the compiler isn't catching it [endless recursion] for whatever reason

well, the recursion wasn't obviously unconditional since it was in the else branch of a condition :)

crates/re_viewport/src/viewport.rs Outdated Show resolved Hide resolved
@Wumpf
Copy link
Member

Wumpf commented Sep 29, 2023

just noticed that the selection behavior I'm complaining was the same in your the description's clip. Somehow it only became unnerving to me when using it....

Anyways, I think the tab container should be greyed out as-if unselected and clicking it should clear the selection instead of keeping the selection on a potentially invisible tab.

@emilk emilk requested a review from Wumpf September 29, 2023 12:40
Copy link
Member

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

actually this is - unlike the "always grey" suggestion I made - perfect now 👍

@Wumpf
Copy link
Member

Wumpf commented Sep 29, 2023

cargo deny failure is a known issue

@Wumpf Wumpf merged commit 6786ca0 into main Sep 29, 2023
26 of 27 checks passed
@Wumpf Wumpf deleted the emilk/fix-tiles-container-tab branch September 29, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🪳 bug Something isn't working 💣 crash crash, deadlock/freeze, do-no-start include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Viewer hangs upon rearranging tiles into view with tabs
2 participants