-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Web canvas resize loop #4699
Comments
Seems like there is some place where we confuse units (logical vs physical pixels). @jprochazk can you take a look? |
Can you provide a repro, e.g. what your HTML/CSS was when embedding rerun in a page? Also note that the ResizeObserver changes haven't even been released yet, so make sure you're testing with the latest |
Sure! Is there a working codesandbox template I could fork to try to repro the issue there? I currently have a custom setup with a local build of the rerun viewer running in a Next.js app, so it would be hard to extract, but I think I should be able to repro it in isolation.
These two lines are where the problem stems from in eframe. |
That doesn't resize the canvas display size, which is the problem you're experiencing. It changes the number of pixels in the canvas drawing buffer. Here's a good writeup about the difference: https://stackoverflow.com/a/43364730 |
In the absence of any CSS explicitly setting the size of the canvas, the width and height attributes of the canvas will also affect its display size: https://codesandbox.io/s/musing-mendeleev-342tww |
Right, so that's a problem, because:
Is there a reason why you can't set |
The original reason was that rerun's |
I agree that it is surprising. We'll have to think about how best to detect this, and document that users are expected to set the canvas size through CSS. As for the rerun viewer, just today we merged a PR that exposes the underlying canvas as a property on the |
### What We now set the web viewer CSS width/height to some default values, which can be modified by the user. Setting them to empty strings is allowed to disable the default values completely, and allow styling the canvas through other means as before. This is meant as a mitigation for emilk/egui#4699 - by setting the CSS width/height to _something_, the canvas width/height attributes won't contribute to the display size of the canvas. Tested in https://github.com/rerun-io/web-viewer-example by manually linking the package ### 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/6636?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/6636?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)! - [PR Build Summary](https://build.rerun.io/pr/6636) - [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`.
How do we document this better? In |
I got pretty badly hit by that when trying to updated my own project from 0.27. Fixed it by applying the following patch to the CSS (which I had originally copied from egui's template): margin-left: auto;
display: block;
position: absolute;
- top: 0%;
- left: 50%;
- transform: translate(-50%, 0%);
+ top: 0;
+ left: 0;
+ width: 100%;
+ height: 100%;
}
.centered { [For discoverability/searchability] This created these warnings in the console:
and cascaded into the following, texture-related crash (as noted by OP):
|
Describe the bug
#4536 introduced a ResizeObserver-based resizing for the canvas. Testing it with rerun, when the size of the canvas isn't explicitly bounded via CSS, the canvas quickly enters a resize loop where:
.observe
. In the callback, the canvas is resized to the measured device pixel size. For instance, if the device pixel ratio is 2 and the canvas size hasn't been explicitly set via CSS, the canvas will be resized from 300x150 (logical pixels) to 600x300 (physical pixels).The text was updated successfully, but these errors were encountered: