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

Elements that should be phetioFeatured #300

Closed
arouinfar opened this issue Jun 16, 2023 · 8 comments
Closed

Elements that should be phetioFeatured #300

arouinfar opened this issue Jun 16, 2023 · 8 comments

Comments

@arouinfar
Copy link
Contributor

For #293

@jbphet here are the remaining phetioFeatured elements. For brevity, I didn't repeat elements that appear on multiple screens, such as energyLegend.visibleProperty, but the desire is that it is featured on all applicable screens. Similarly, decisions made about Layer0 apply to all layers. Please let me know if you run into any questions.

/* eslint-disable */
window.phet.preloads.phetio.phetioElementsOverrides =
  {
    "greenhouseEffect.global.model.preferences.cueingArrowsEnabledProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.model.atmosphereLayers.numberOfActiveAtmosphereLayersProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.model.layersInfraredAbsorbanceProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.model.sunEnergySource.proportionateOutputRateProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.view.infraredPanel.visibleProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.view.observationWindow.atmosphereLayer0.showTemperatureProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.view.observationWindow.atmosphereLayer0.temperatureReadout.valueText.stringProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.view.observationWindow.fluxMeterNode.zoomButtonGroup.visibleProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.view.observationWindow.fluxMeterNode.zoomFactorProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.layerModelScreen.view.sunlightPanel.visibleProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.photonsScreen.model.fluxMeter.fluxMeterVisibleProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.photonsScreen.model.fluxMeter.fluxSensor.altitudeProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.photonsScreen.model.photonCollection.showAllSimulatedPhotonsInViewProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.cloud.cloudEnabledInManualConcentrationModeProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.cloud.cloudEnabledProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.energyBalance.inRadiativeBalanceProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.energyBalance.netInflowOfEnergyProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.groundLayer.albedoProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.groundLayer.atEquilibriumProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.surfaceTemperature.surfaceTemperatureCelsiusProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.surfaceTemperature.surfaceTemperatureFahrenheitProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.view.energyLegend.visibleProperty": {
      "phetioFeatured": true
    }
  };
@arouinfar
Copy link
Contributor Author

I would also really like to unfeature the enabledProperty and visibleProperty of the individual radio buttons in the concentrationControlModeRadioButtonGroup. However, they are featured in sun, not the sim, so I have no idea if this is even possible.

image

Disabling or hiding these buttons individually is nonsensical, but we punted on other options in phetsims/sun#826.

@pixelzoom
Copy link
Contributor

I'll take a look. If #300 (comment) is not currently possible, I'll create a sun issue. But this should not block Greenhouse.

If you can disable individual radio buttons, that seems like a bug.

@pixelzoom pixelzoom assigned pixelzoom and unassigned jbphet Jun 19, 2023
pixelzoom added a commit that referenced this issue Jun 19, 2023
@pixelzoom
Copy link
Contributor

For the request in #300 (comment), unfeaturing visibleProperty was easy, done in e95977b. enabledProperty should be able to be uninstrumented similarly, but it's not working -- setting phetioFeature, phetioDocumentation, or other metadata has no affect on the radio button's enabledProperty. I suspect this is a common-code problem, and I'll need to investigate further.

@pixelzoom
Copy link
Contributor

pixelzoom commented Jun 19, 2023

Questions for @arouinfar about a few of the phetioFeatured requests:

(1) temperatureReadout.valueText.stringProperty is inside NumberDisplay (common code), with no programming API to customize it. How badly do you want this?

   "greenhouseEffect.layerModelScreen.view.observationWindow.atmosphereLayer0.temperatureReadout.valueText.stringProperty": {
      "phetioFeatured": true
    },

(2) cloudEnabledProperty is derived, and therefore read-only. Are you sure that you want to feature it?

    "greenhouseEffect.wavesScreen.model.cloud.cloudEnabledProperty": {
      "phetioFeatured": true
    },

(3) These elements are phetioHighFrequency: true and derived (read-only). Are you sure you want to feature them?

    "greenhouseEffect.wavesScreen.model.surfaceTemperature.surfaceTemperatureCelsiusProperty": {
      "phetioFeatured": true
    },
    "greenhouseEffect.wavesScreen.model.surfaceTemperature.surfaceTemperatureFahrenheitProperty": {
      "phetioFeatured": true
    },

@arouinfar
Copy link
Contributor Author

Questions for @arouinfar about a few of the phetioFeatured requests:

Good questions, @pixelzoom. I realize that none of these elements can be used to make customizations in Studio, so thanks for checking in. I wanted to bring attention to some of the properties that I think could be useful in the datastream or data students may collect.

(1) temperatureReadout.valueText.stringProperty is inside NumberDisplay (common code), with no programming API to customize it. How badly do you want this?

I don't want it badly enough to go through any hoops, so let's skip it. The idea was that clients could use this to populate a data table with values that exactly match what students are seeing in the view. It's not critical, they can just round the model values instead.

(2) cloudEnabledProperty is derived, and therefore read-only. Are you sure that you want to feature it?

Yes, I do. This is a one-stop shop if clients want to know if the cloud is enabled. Otherwise, they need to check both dependencies themselves.

(3) These elements are phetioHighFrequency: true and derived (read-only). Are you sure you want to feature them?

Yes, these are convenience properties that are useful for populating a data table. They definitely have a lot of noise, but I would imagine that clients would round values collected this way.

pixelzoom added a commit that referenced this issue Jun 19, 2023
@pixelzoom
Copy link
Contributor

... I don't want it badly enough to go through any hoops, so let's skip it.

🎉

All phetioFeatured requests are completed. Back to @arouinfar to review, close if OK.

@pixelzoom
Copy link
Contributor

PS: I moved the problem with enabledProperty for the individual radio buttons to phetsims/sun#847. So if everything else looks OK (including visibleProperty for those radio buttons), feel free to close this issue.

@arouinfar
Copy link
Contributor Author

Thanks @pixelzoom. Everythings looks good here, 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

3 participants