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 Window positioning bug when bad pivot is stored in app data #3721

Merged
merged 1 commit into from Dec 23, 2023

Conversation

abey79
Copy link
Collaborator

@abey79 abey79 commented Dec 19, 2023

This PR fixes an issue where a bad pivot stored in app state would override the pivot actually set from code. The root code of this issue if the Window's State containing pivot in the first place, as pivot is not part of a state to be tracked. It makes the State structure more ergonomic though, so this PR leaves the pivot field but always overrides it with the value provided by user code.

Repro of the original issue

This issue can be reproduced using the re_ui_example app from the Rerun repo, at this commit: rerun-io/rerun@fb5add0. By using this app.ron file, the bug will appear:

{
    "egui": "(
      areas:{
            ((0)):(
                areas:{
                    (13430889033718688666):(pivot_pos:(x:565.5,y:328.0),pivot:((Min,Min)),size:(x:364.0,y:75.5),interactable:true),
                },
            )
        }
    )",
}

The modal is entered based on it's top-left corner even though the code actually specifies a center-center pivot:

image

With this PR, the centring is correct even with the "poisoned" app.ron:

image

@abey79 abey79 added bug Something is broken egui labels Dec 19, 2023
@abey79
Copy link
Collaborator Author

abey79 commented Dec 19, 2023

The fact that persisted app state can corrupt State makes me think that this "TODO" comment might be incorrect:

/// State that is persisted between frames.
// TODO(emilk): this is not currently stored in `Memory::data`, but maybe it should be?
#[derive(Clone, Copy, Debug)]
#[cfg_attr(feature = "serde", derive(serde::Deserialize, serde::Serialize))]
pub(crate) struct State {
/// Last known pos of the pivot
pub pivot_pos: Pos2,
pub pivot: Align2,
/// Last know size. Used for catching clicks.
pub size: Vec2,
/// If false, clicks goes straight through to what is behind us.
/// Good for tooltips etc.
pub interactable: bool,
}

abey79 added a commit to rerun-io/rerun that referenced this pull request Dec 19, 2023
…w entity picker (#4577)

### What

Since egui doesn't have support for modal windows yet (emilk/egui#686),
we rolled our own for the Space View entity picker ("Add/remove
entities"). This PR abstracts the modal creation code into a dedicated
feature in `re_ui`, and use that abstraction for the space view entity
picker.

The new `re_ui::modal` has support for two aspects of modal window
management:
- The modal window itself (`re_ui::modal::Modal`), which handles dimming
the background, a close button, and exiting with ESC and/or clicking
outside of the window.
- The handling of the modal window (`re_ui::modal::ModalHandler`). The
`Modal` struct should be kept only while the modal is actually shown, so
it's typically held in and `Option<Modal>`. `ModalHandler` implements
the house keeping of populating the `Option` when the modal must be
displayed, and dropping it when the modal is closed.

**Note**: with this PR, the modals are now properly centred, although
erroneous centring behaviour might happen that require deleting your
`app.ron`. This will eventually be fixed with a new egui release that
includes emilk/egui#3721

Usage:

<img width="583" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/a079def4-f11a-4eeb-a3ec-6c777d078bdd">


<img width="443" alt="image"
src="https://github.com/rerun-io/rerun/assets/49431240/13114dd4-3051-486c-9e4a-df4c783a0780">



### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4577/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4577/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4577/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4577)
- [Docs
preview](https://rerun.io/preview/fb5add0047f108769c413393d65d52e362cbcc8e/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/fb5add0047f108769c413393d65d52e362cbcc8e/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)

---------

Co-authored-by: Andreas Reich <r_andreas2@web.de>
@emilk
Copy link
Owner

emilk commented Dec 23, 2023

The fact that persisted app state can corrupt State makes me think that this "TODO" comment might be incorrect:

The comment is correct. The area state is not stored in Memory::data, but in another part of Memory (so it is still persisted). Functionally this matters little, it is just inconsistent with other widget state.

@emilk emilk merged commit 365a8d2 into master Dec 23, 2023
37 checks passed
@emilk emilk deleted the antoine/fix-bad-pivot branch December 23, 2023 14:53
abey79 added a commit to rerun-io/rerun that referenced this pull request Jan 5, 2024
…e window (#4700)

### What

This avoids the modal moving vertically when content size changes.

**NOTE**: this PR would trigger the bug fixed in
emilk/egui#3721, so it needs an egui release
before being released. This is avoided by salting the modals' ID such as
to in invalidate existing caches.


https://github.com/rerun-io/rerun/assets/49431240/d2d472b5-7af2-4e95-9da1-a671fc80629f


### Checklist
* [x] I have read and agree to [Contributor
Guide](https://github.com/rerun-io/rerun/blob/main/CONTRIBUTING.md) and
the [Code of
Conduct](https://github.com/rerun-io/rerun/blob/main/CODE_OF_CONDUCT.md)
* [x] I've included a screenshot or gif (if applicable)
* [x] I have tested the web demo (if applicable):
* Using newly built examples:
[app.rerun.io](https://app.rerun.io/pr/4700/index.html)
* Using examples from latest `main` build:
[app.rerun.io](https://app.rerun.io/pr/4700/index.html?manifest_url=https://app.rerun.io/version/main/examples_manifest.json)
* Using full set of examples from `nightly` build:
[app.rerun.io](https://app.rerun.io/pr/4700/index.html?manifest_url=https://app.rerun.io/version/nightly/examples_manifest.json)
* [x] The PR title and labels are set such as to maximize their
usefulness for the next release's CHANGELOG

- [PR Build Summary](https://build.rerun.io/pr/4700)
- [Docs
preview](https://rerun.io/preview/e159b005a7a1d5f44e608013f00c1d479b7c341c/docs)
<!--DOCS-PREVIEW-->
- [Examples
preview](https://rerun.io/preview/e159b005a7a1d5f44e608013f00c1d479b7c341c/examples)
<!--EXAMPLES-PREVIEW-->
- [Recent benchmark results](https://build.rerun.io/graphs/crates.html)
- [Wasm size tracking](https://build.rerun.io/graphs/sizes.html)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken egui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants