Create a reusable GridNode
Closed this issue · 22 comments
From phetsims/energy-skate-park#205. There are many sims that have very similar grids, each with similar implementations. It would be beneficial to have something for common code for this.
Most of the grids look very similar, they have major (thick) lines and minor (thin) lines.
charges-and-fields:
collision-lab:
equality-explorer: (for debugging)
gravity-and-orbits:
states-of-matter
PlayAreaGridNode (BASE) is very different and wouldn't benefit from this. GridLinesNode (fluid-pressure-and-flow), ISLCGridNode (inverse-square-law-common) are only lines ind one direction and probably wouldn't use this. bending-light has a grid-canvas node but I Idon't want to include that as it uses canvas for performance.
This is awesome, thanks for doing this work! I'm very interested in using this for phetsims/ratio-and-proportion#5. Let me know if the API changes greatly, and I'll be happy to update Proportion.
I created the GridNode in 2b26428 and it is being used in energy-skate-park and ratio-and-proportion.
Over in phetsims/ratio-and-proportion#5 @zepumph asked
I haven't yet gotten to adding numbers on the grid, in part because I wonder if that support will be added to the common code Node at some point.
Most grids don't have labels, and it seemed to me that many grid labels would be too different from sim to sim to be added generally. But @zepumph recommended that we revisit this and we brainstormed having a callback that could be used to add labels to horizontal and veritcal lines with custom positioning. Ill try it out.
EDIT: For instance, maybe an optional function like
addLabel: ( isMajor, lineOrientation, linePosition, options ) => {
// create a label based on the provided information, and offset/positioned with options
return null;
},Thinking through an optional callback more...lets say I want labels along the bottom of my grid along the major vertical lines - These could be added with the following:
const bottom = gridNode.bottom;
for ( let x = 0; x < gridWidth; x += majorSpacing ) {
gridNode.addChild( new Text( x, { x: x, top: bottom } ) );
}Trying to accomplish the same thing with a callback, assuming the callback is called at each line intersection, might look like:
labelOptions: {
// ...
},
addLabel: ( isMajor, orientation, linePosition, labelOptions ) => {
let label = null;
if ( isMajor && orientation === Orientations.VERTICAL ) {
if ( linePosition.y === gridHeight ) {
label = new Text( linePosition.x )
label.mutate( labelOptions );
return label;
}
}
return label;
}I still think that decoration is the simplest way to labels to a GridNode.
This is ready for a review. It was motivated by energy-skate-park, but could be reviewed by any developer. @ariel-phet could you please assign someone?
For the reviewer: This is a new component for scenery-phet that could be used in the examples listed in #580 (comment). For usage examples see EnergySkateParkGridNode and scenery-phet/ComponentsScreenView.
@zepumph seems natural since he has been using this feature.
Thanks for being a reviewer @zepumph! Also linking review comments made by @pixelzoom in #601
Sorry for doing an informal review, I didn't know this issue existed. Does it makes sense to address my review comments before @zepumph does another review?
Yea, thanks, lets do #601 first.
In #601 @pixelzoom reminded that new components should be PhET-iO instrumented, and that it would be desirable to pass Tandems to the major/minor grid line Paths. But when I asked about Tandems for GridNode @zepumph said that he didn't think Tandems were needed for those and that if instrumentation is desired it can be done at the usage of the GridNode.
- @zepumph when you review this can you also review this comment and the decision for omitting iO instrumentation?
But when I asked about Tandems for GridNode @zepumph said that he didn't think Tandems were needed for those and that if instrumentation is desired it can be done at the usage of the GridNode.
My assumption is that designers will want to be able to control the visibility of major and minor grid lines independently. That's been the case in every graph that I've instrumented so far, but confirm with a designer. How can that be accomplished without instrumenting majorLines and minorLines?
@jessegreenberg CT is failing with:
scenery-phet : fuzz : require.js
https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/scenery-phet/scenery-phet_en.html?continuousTest=%7B%22test%22%3A%5B%22scenery-phet%22%2C%22fuzz%22%2C%22require.js%22%5D%2C%22snapshotName%22%3A%22snapshot-1587854501239%22%2C%22timestamp%22%3A1587970818472%7D&brand=phet&ea&fuzz&memoryLimit=1000
Query: brand=phet&ea&fuzz&memoryLimit=1000
Uncaught Error: Assertion failed: At least one line should be specified
Error: Assertion failed: At least one line should be specified
at window.assertions.assertFunction (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/assert/js/assert.js:22:13)
at GridNode.setLineSpacings (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/scenery-phet/js/GridNode.js:90:15)
at https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/scenery-phet/js/demo/ComponentsScreenView.js:607:14
at listener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/axon/js/Multilink.js:38:18)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/axon/js/TinyEmitter.js:69:53)
at BooleanProperty._notifyListeners (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/axon/js/Property.js:270:25)
at BooleanProperty.set (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/axon/js/Property.js:170:14)
at BooleanProperty.set value [as value] (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/axon/js/Property.js:340:32)
at toggleListener (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/sun/js/buttons/ToggleButtonModel.js:60:30)
at TinyEmitter.emit (https://bayes.colorado.edu/continuous-testing/ct-snapshots/1587854501239/axon/js/TinyEmitter.js:69:53)
id: Bayes Chrome
Thanks, issue with the demo fixed in f6ad5f1
Everything here is looking really good to me. As for the PhET-iO instrumentation of this component, it seems best to check with a designer.
@arouinfar can you comment on if, in a GridNode, you should be able to control the minor and major grid lines independently, thus needing to instrument each individually? @pixelzoom mentioned that this was the design spec in grid instrumenting cases he has encountered in the past.
Are there any other phet-io requirements for GridNode?
@arouinfar can you comment on if, in a GridNode, you should be able to control the minor and major grid lines independently, thus needing to instrument each individually? @pixelzoom mentioned that this was the design spec in grid instrumenting cases he has encountered in the past.
Theoretically, I understand wanting to hide minor gridlines. However, this isn't a great idea in practice. Minor gridlines exist because they serve a purpose in a particular sim, and I think the grid would become far less usable without them.
I don't think we would want users to turn off the major gridlines, and I definitely wouldn't feature their visibleProperty.
It might also be nice for @amanda-phet to chime in here.
My assumption is that designers will want to be able to control the visibility of major and minor grid lines independently. That's been the case in every graph that I've instrumented so far, but confirm with a designer.
@pixelzoom can you tell me which sims this has been important for? My initial assumption was that you may have been referring to Graphing Quadratics, but its grid hasn't been instrumented.
You're right - we didn't instrument the graph in Graphing Quadratics. If you say it's not important to instrument minor and major grid lines, that's fine by me.
I haven't done many sims with grids, but from reading this issue and considering some of those sims, here are my thoughts:
- Cartesian coordinate grids in math sims have typically included a checkbox to turn the grid on/off, so that's probably why we didn't instrument the grid in GQ. As long as this checkbox exists, it seems like an iO designer could use that to choose whether the gridlines are visible or not.
- We would never want to instrument major axes, I would think, because those are critical in a graphing sim.
- If a sim has major and minor gridlines, I could go either way on iO instrumentation... Either instrument the entire grid as one element, or potentially the minor gridlines separately. I just don't have enough context to have an opinion.
It seems like the consensus is slightly in favor of not instrumenting the minor and major lines separately, but instead only the whole gridNode. Thus it seems appropriate to not add any instrumentation to this, and to just rely on Node to add an instrumented visibleProperty if we are instrumenting this Node. If any disagree, speak now to further the discussion. Back to @jessegreenberg.
Please remove the hold from phetsims/gravity-and-orbits#328 when this issue is complete.
According to #580 (comment) no further instrumentation is required for this Node and that was the last part of this issue. Otherwise review was complete in #580 (comment).