Skip to content

feat(RenderingEngine): Make reset camera behavior configurable #1923

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

KlimMixer
Copy link
Contributor

@KlimMixer KlimMixer commented Mar 20, 2025

Context

Closes #1922

Changes & Results

Add configuration for resetCameraForResizez so client application can configure behavior how RenderEngine.resize should reset camera.
Add configuration for setOrientation so client application can decide whether to reset camera on orientation change or not.
All default behaviors are kept the same as v2.0 default behaviors, so clients that don't need them won't be affected.

Testing

Checklist

PR

  • My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

  • My code has been well-documented (function documentation, inline comments,
    etc.)

Public Documentation Updates

  • The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • "OS: Windows 11"
  • "Node version: v22.14.0"
  • "Browser: Chrome 134.0.6998.89"

Copy link

netlify bot commented Mar 20, 2025

Deploy Preview for cornerstone-3d-docs failed. Why did it fail? →

Name Link
🔨 Latest commit cc3aa6f
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/67dbdfff2851500008f38da5

@sedghi
Copy link
Member

sedghi commented Mar 26, 2025

I'm not really on board with this change.

It works fine in OHIF, so how we handle double-clicking is likely an app-level concern.

In OHIF, we're probably getting the viewPresentation via viewport.getViewPresentation, and then applying it after a double-click to preserve it.

We should not mix the resize with camera stuff

CleanShot.2025-03-26.at.13.49.51.mp4

@KlimMixer
Copy link
Contributor Author

KlimMixer commented Mar 26, 2025

Yes, it makes sense to handle camera settings at the application level. However, there is already a mix of reset camera call at viewport resize. And because of that, I thought it might be useful to make it configurable. With the configurable approach there will be less renderings, because at resize step render is called, and at apply viewPresentation there is a second call to render. With the configurable approach we don't need to render twice, it's just one render on resize. This also removes the glitch that after resizing the user can see a resized zoom/pan for one or two frames.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Make reset camera behavior configurable
2 participants