simularium / simularium-viewer

NPM package to view Simularium trajectories in 3D
Apache License 2.0
2 stars 0 forks source link

Feature/freeze camera #363

Closed meganrm closed 6 months ago

meganrm commented 7 months ago

Problem

The education module will not need camera controls because it's a 2d simulation

Solution

I added a prop called stationaryViewport that defaults to false.

NOTES/QUESTIONS:

  1. I made the assumption here that this setting is unlikely to change for a simulation. It's set when the canvas is mounted and nothing would happen if the prop changed but the canvas isn't remounted. I can add the ability to have it react to the prop change, but it's not needed for the education simulation and so I'm partial to waiting until it's a requirement for something else but open to changing if other people feel differently.
  2. any ideas for a clearer prop name are welcome. I could also see the argument for having a movableViewport prop that defaults to true

Type of change

Please delete options that are not relevant.

Steps to Verify:

  1. npm start
  2. change stationaryViewport to true in the example viewer
  3. camera controls shouldn't work and nothing should happen when you click on an agent.
github-actions[bot] commented 7 months ago

jest coverage report πŸ§ͺ

Total coverage

Status Category Percentage Covered / Total
πŸ”΄ Statements 40.48% 2044/5049
πŸ”΄ Branches 43.35% 845/1949
πŸ”΄ Functions 36.9% 417/1130
πŸ”΄ Lines 40.69% 1957/4809

Status of coverage: 🟒 - ok, 🟑 - slightly more than threshold, πŸ”΄ - under the threshold

Show files with reduced coverage πŸ”» ### Reduced coverage | Status | Filename | Statements | Branches | Functions | Lines | | :----: | :-------------- | :--------- | :------- | :-------- | :------ | | πŸ”΄ | index.ts | 7.76% | 7.37% | 5.78% | 7.73% | | πŸ”΄ | src/viewport | 15.27% | 10.95% | 15.78% | 14.81% | | πŸ”΄ | index.tsx | 15.27% | 10.95% | 15.78% | 14.81% | | πŸ”΄ | src/visGeometry | 21.89% | 22.95% | 27.92% | 21.78% | > Status of coverage: 🟒 - ok, 🟑 - slightly more than threshold, πŸ”΄ - under the threshold
interim17 commented 7 months ago

LGTM, left some ideas about future work on configuring controls.

I don't mind stationaryViewport but I might prefer freezeCamera,disableCameraMovement, fixedCameraPosition(and flip the boolean) or something like that, since we are preventing camera movement within the viewport. Not a strong preference though, fine with me to leave as is.

toloudis commented 7 months ago

I saw that you don't allow the "follow" behavior. Do you still want to allow mouse clicking to pick an agent?

toloudis commented 7 months ago

I don't mind stationaryViewport but I might prefer freezeCamera,disableCameraMovement, fixedCameraPosition(and flip the boolean) or something like that, since we are preventing camera movement within the viewport. Not a strong preference though, fine with me to leave as is.

or maybe lockedCamera...

meganrm commented 6 months ago

I saw that you don't allow the "follow" behavior. Do you still want to allow mouse clicking to pick an agent?

It doesn't really matter if an agent is "picked" as long as we're not following it. For completeness are you saying I should also make it so click on an agent doesn't save the id?

toloudis commented 6 months ago

I saw that you don't allow the "follow" behavior. Do you still want to allow mouse clicking to pick an agent?

It doesn't really matter if an agent is "picked" as long as we're not following it. For completeness are you saying I should also make it so click on an agent doesn't save the id?

I don't mind either way but it can result in the agent being selected and outlined. Just want to make sure we are intentional. My comment was just about being able to document (in the code in comments near where the prop is defined, or something) to know exactly what is supposed to be locked down when this prop is set.