Add Context::request_discard for multi-pass layouts
#5059
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
emilk
force-pushed
the
emilk/skip-frame
branch
from
ece336c
to
9b156a3
Compare
emilk
force-pushed
the
emilk/skip-frame
branch
from
b71a2f1
to
6a06c98
Compare
emilk
force-pushed
the
emilk/skip-frame
branch
from
2c60a7f
to
b01a7b1
Compare
emilk
changed the title
Context::request_discardContext::request_discard for multi-pass layouts
6 tasks
|
Very simple and elegant solution. I appreciate the guardrails too (frame limit and warning) 👌 |
Wumpf
pushed a commit
to rerun-io/rerun
that referenced
this pull request
Sep 16, 2024
### What Uses the latest egui containing this gem: * emilk/egui#5059 So far `request_discard` (running a second pass) is only used in these situations: * For when a `Grid` first shows up * When an egui_table first shows up * When asked to auto-size an `egui_table` row Of course, running a second pass has some costs. Luckily, we already execute our most expensive stuff (the viewport) last, and so we can easily early-out from things like point cloud upload/rendering, as shown in this puffin-capture of a frame with two passes in it:  NOTE: this flamegraph is of a DEBUG build - the point is not the relative times, but that `pass 0` becomes very cheap, if the `request_discard` is called from one of the non-viewport panels (e.g. the selection panel). There might still be cases where we do expensive stuff twice, e.g. if something calls `request_discard` in a space view tooltip. If we come to regret this, we can disable all second passes by setting `egui::Options::max_passes` to `1`. ### 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 examples from latest `main` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7418?manifest_url=https://app.rerun.io/version/main/examples_manifest.json) * Using full set of examples from `nightly` build: [rerun.io/viewer](https://rerun.io/viewer/pr/7418?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 * [x] If applicable, add a new check to the [release checklist](https://github.com/rerun-io/rerun/blob/main/tests/python/release_checklist)! * [x] If have noted any breaking changes to the log API in `CHANGELOG.md` and the migration guide - [PR Build Summary](https://build.rerun.io/pr/7418) - [Recent benchmark results](https://build.rerun.io/graphs/crates.html) - [Wasm size tracking](https://build.rerun.io/graphs/sizes.html) To run all checks from `main`, comment on the PR with `@rerun-bot full-check`.
hacknus
pushed a commit
to hacknus/egui
that referenced
this pull request
Oct 30, 2024
* Closes emilk#4976 * Part of emilk#4378 * Implements parts of emilk#843 ### Background Some widgets (like `Grid` and `Table`) needs to know the width of future elements in order to properly size themselves. For instance, the width of the first column of a grid may not be known until all rows of the grid has been added, at which point it is too late. Therefore these widgets store sizes from the previous frame. This leads to "first-frame jitter", were the content is placed in the wrong place for one frame, before being accurately laid out in subsequent frames. ### What This PR adds the function `ctx.request_discard` which discards the visual output and does another _pass_, i.e. calls the whole app UI code once again (in eframe this means calling `App::update` again). This will thus discard the shapes produced by the wrongly placed widgets, and replace it with new shapes. Note that only the visual output is discarded - all other output events are accumulated. Calling `ctx.request_discard` should only be done in very rare circumstances, e.g. when a `Grid` is first shown. Calling it every frame will mean the UI code will become unnecessarily slow. Two safe-guards are in place: * `Options::max_passes` is by default 2, meaning egui will never do more than 2 passes even if `request_discard` is called on every pass * If multiple passes is done for multiple frames in a row, a warning will be printed on the screen in debug builds:  ### Breaking changes A bunch of things that had "frame" in the name now has "pass" in them instead: * Functions called `begin_frame` and `end_frame` are now called `begin_pass` and `end_pass` * `FrameState` is now `PassState` * etc ### TODO * [x] Figure out good names for everything (`ctx.request_discard`) * [x] Add API to query if we're gonna repeat this frame (to early-out from expensive rendering) * [x] Clear up naming confusion (pass vs frame) e.g. for `FrameState` * [x] Figure out when to call this * [x] Show warning on screen when there are several frames in a row with multiple passes * [x] Document * [x] Default on or off? * [x] Change `Context::frame_nr` name/docs * [x] Rename `Context::begin_frame/end_frame` and deprecate the old ones * [x] Test with Rerun * [x] Document breaking changes
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
Some widgets (like
GridandTable) needs to know the width of future elements in order to properly size themselves. For instance, the width of the first column of a grid may not be known until all rows of the grid has been added, at which point it is too late. Therefore these widgets store sizes from the previous frame. This leads to "first-frame jitter", were the content is placed in the wrong place for one frame, before being accurately laid out in subsequent frames.What
This PR adds the function
ctx.request_discardwhich discards the visual output and does another pass, i.e. calls the whole app UI code once again (in eframe this means callingApp::updateagain). This will thus discard the shapes produced by the wrongly placed widgets, and replace it with new shapes. Note that only the visual output is discarded - all other output events are accumulated.Calling
ctx.request_discardshould only be done in very rare circumstances, e.g. when aGridis first shown. Calling it every frame will mean the UI code will become unnecessarily slow.Two safe-guards are in place:
Options::max_passesis by default 2, meaning egui will never do more than 2 passes even ifrequest_discardis called on every passBreaking changes
A bunch of things that had "frame" in the name now has "pass" in them instead:
begin_frameandend_frameare now calledbegin_passandend_passFrameStateis nowPassStateTODO
ctx.request_discard)FrameStateContext::frame_nrname/docsContext::begin_frame/end_frameand deprecate the old ones