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

moving constantRadiusProperty to ISLCModel #38

Closed
mbarlow12 opened this issue Jan 19, 2018 · 2 comments
Closed

moving constantRadiusProperty to ISLCModel #38

mbarlow12 opened this issue Jan 19, 2018 · 2 comments

Comments

@mbarlow12
Copy link
Contributor

mbarlow12 commented Jan 19, 2018

@samreid I noticed your TODO in GFLBModel.js regarding moving the constant radius to ISLCModel. I agree that this (along with the baseColors) would simplify a lot of the code in all the ISLC sims. My concern is that, since it's instrumented or PhetIO, offering that API in Coulomb's Law doesn't really make any sense as there's no way to update the radius were that property set to true.

Is there a way to conditionally set it as a read-only property in CL and have it fully instrumented in GFL and GFLB?

@samreid
Copy link
Member

samreid commented Jan 19, 2018

Sure, you can pass phetioReadOnly as an option to the Property. But I noticed the general flow is:

    this.constantRadiusProperty = new Property( false, {
      tandem: tandem.createTandem( 'constantRadiusProperty' ),
      phetioType: PropertyIO( BooleanIO )
    } );

 // ...
    var mass1 = new Mass( value1, position1, valueRange, density, this.constantRadiusProperty, baseColor1, tandem.createTandem( 'mass1' ), massOptions );
    var mass2 = new Mass( value2, position2, valueRange, density, this.constantRadiusProperty, baseColor2, tandem.createTandem( 'mass2' ), massOptions );

    ISLCModel.call( this, ISLCConstants.G, mass1, mass2, leftBoundary, rightBoundary, tandem, {
      snapObjectsToNearest: GravityForceLabBasicsConstants.MASS_POSITION_DELTA,
      minSeparationBetweenObjects: 200 // in meters
    } );

So it wouldn't be a trivial change to move this Property to ISLCModel, because the Property is needed before creating the masses, and the masses are needed before creating the ISLCModel. So we may wish to leave this as it is (or it would require a more invasive change).

@samreid samreid assigned mbarlow12 and unassigned samreid Jan 19, 2018
@mbarlow12
Copy link
Contributor Author

mbarlow12 commented Jan 19, 2018

Good point—I'd forgotten about that element. I think I'll opt for leaving as is.

@samreid unless you have objections, I'll close and remove the TODO for now.

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

No branches or pull requests

2 participants