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 disappearing cached shapes #6458

Merged
merged 8 commits into from
May 1, 2023
Merged

Fix disappearing cached shapes #6458

merged 8 commits into from
May 1, 2023

Conversation

kazcw
Copy link
Contributor

@kazcw kazcw commented Apr 27, 2023

Pull Request Description

Fix cached-shapes disappearance when resizing window or moving to differently-shaped monitor (#6125).

The bug:
CacheShapesPass relied on old behavior of display_object.update(..): The update used to be finished when the call returned, but now it spawns an asynchronous task. It is now necessary to schedule a task to update the layer's camera after the scene's display object update has propagated its transformation matrix to the pass's layer.

Additional optimization:

  • Don't re-render cached shapes when the canvas size changes.

Some cleanups:

  • Fix dangling GL texture objects.
  • Fix GL framebuffer leak.
  • Fix mixed-up GL texture parameters.

Important Notes

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • The documentation has been updated, if necessary.
  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed, the GUI was tested when built using ./run ide build.

@kazcw kazcw self-assigned this Apr 27, 2023
@kazcw kazcw added the CI: No changelog needed Do not require a changelog entry for this PR. label Apr 27, 2023
@kazcw kazcw marked this pull request as ready for review April 27, 2023 16:35
Comment on lines 115 to 118
if !self.camera_ready.get() {
// We cannot render before the camera's transformation matrix has been set.
return;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it won't hurt to make if-else instead of early return.

@kazcw kazcw added the CI: Ready to merge This PR is eligible for automatic merge label Apr 28, 2023
Comment on lines +96 to +104
let handle = frp::microtasks::next_microtask({
let camera = self.layer.camera();
let scene = self.scene.clone_ref();
let camera_ready = Rc::clone(&self.camera_ready);
move || {
camera.update(&scene);
camera_ready.set(true);
}
});
Copy link
Member

@wdanilo wdanilo Apr 28, 2023

Choose a reason for hiding this comment

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

Hmm, we are updating cameras in Scene::update_layout How does this code relate to the update functions in Scene? Everything is probably fine, but I do not understand yet why we are handling the same thing in two places (or maybe my knowledge about the (EnsoGL) world is not up to date now ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The update finishes asynchronously now, due to the batch FRP used in layer updates. Other layers work OK because we poll their dirty flags unconditionally when processing all updates, so they eventually see the results of all microtasks--but this layer is only updated once, and its draws only occur once, so we need to know when it's ready.

Comment on lines -173 to +176
let internal_format = texture::Rgba;
let internal_format = texture::Rgba8;
Copy link
Member

Choose a reason for hiding this comment

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

Wait, if we've been using wrong format, how on earth was it working? :O

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Rgba isn't wrong; used with u8 channel values, it is usually equivalent to using Rgba8. However, in some situations the more specific Rgba8 value can produce a complete texture, when Rgba would not; and there are driver (and browser)-specific differences between handling of incomplete textures... So, although there should be more difference here, I think it is safest to provide the most specific value possible.

@kazcw kazcw removed the CI: Ready to merge This PR is eligible for automatic merge label Apr 28, 2023
@kazcw kazcw merged commit a839545 into develop May 1, 2023
@kazcw kazcw deleted the wip/kw/fix-resize branch May 1, 2023 14:30
kazcw added a commit that referenced this pull request May 1, 2023
Procrat added a commit that referenced this pull request May 2, 2023
* develop:
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
Procrat added a commit that referenced this pull request May 3, 2023
* develop: (34 commits)
  Continued Execution Context work and some little fixes (#6506)
  IDE's logging to a file (#6478)
  Fix application config (#6513)
  Cloud/desktop mode switcher (#6448)
  Fix doubled named arguments bug (#6422)
  Reimplement `enso_project` as a proper builtin (#6352)
  Fix layer ordering between dropdown and breadcrumbs backgrounds.  (#6483)
  Multiflavor layers (#6477)
  DataflowAnalysis preserves dependencies order (#6493)
  Implement `create_database_table` for Database Table (#6467)
  Limit Dead Letter logging (#6482)
  More reliable shutdown of the EnsoContext to save resources (#6468)
  Make execution mode `live` default for CLI (#6496)
  Finishing Vector Editor (#6470)
  Proper handling of multiple list views. (#6461)
  Fix disappearing cached shapes (#6458)
  Add Execution Context control to Text.write (#6459)
  Change defaults for `Connection.tables` and ensure that `Connection.query` recognizes all available tables (#6443)
  Introducing @BuiltinMethod.needsFrame and InlineableNode (#6442)
  Hide profile button behind a feature flag (#6430)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Component Browser Icons are not on an external screen with different zoom level in Windows 11 Pro
3 participants