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

Light beam still visible when laser is hidden #310

Closed
arouinfar opened this issue Jan 12, 2023 · 5 comments
Closed

Light beam still visible when laser is hidden #310

arouinfar opened this issue Jan 12, 2023 · 5 comments

Comments

@arouinfar
Copy link
Contributor

Discovered while reviewing the phetioFeatured overrides for #305. Semi-related to #296.

If you set view.lightNode.visibleProperty to false while the light is on, the laser is hidden but the beam is still visible. This looks buggy to me.
image

Should we update model.light.isOnProperty to hide the beam in this situation? Clients would need to turn the light back on when they make the laser visible in the view again, but that seems reasonable.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jan 12, 2023

In general, I'm not a fan of visibleProperty having side-effects like you've proposed. Could we just make view.lightNode.visibleProperty be phetioReadOnly: true? Is there any reason why the light should be hideable? This screen seems totally useless when there is no light.

@pixelzoom pixelzoom assigned arouinfar and unassigned pixelzoom Jan 12, 2023
@arouinfar
Copy link
Contributor Author

Good point @pixelzoom. If clients want to prevent students from turning on the laser, they should just hide its on/off button. Let's make view.lightNode.visibleProperty be phetioReadOnly: true and phetioFeatured: false. I'll take care of the latter, back to you for the former.

@arouinfar
Copy link
Contributor Author

Updated the overrides in the above commit.

@arouinfar arouinfar removed their assignment Jan 13, 2023
@pixelzoom
Copy link
Contributor

It occurred to me that we typically only instrument visibleProperty with phetioReadOnly: false when the visibility can actually change, and there's some value in the client being able to see if it's changed. That's not the case here, so I uninstrumented lightNode.visibleProperty (via option phetioVisiblePropertyInstrumented: false) and it therefore no longer appears in Studio or the API.

Back to @arouinfar for review.

@arouinfar
Copy link
Contributor Author

Thanks @pixelzoom. The tree looks good, closing.

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

No branches or pull requests

2 participants