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

Address missing tandems #281

Closed
pixelzoom opened this issue Nov 2, 2023 · 9 comments
Closed

Address missing tandems #281

pixelzoom opened this issue Nov 2, 2023 · 9 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

pixelzoom commented Nov 2, 2023

Running Studio with &phetioPrintMissingTandems currently reports that 420 required tandems are missing, and 2509 optional tandems are missing. I'm having a lot of trouble understanding the console output, to identify where I need to add tandems. I'll need synchronous help from either @samreid or @zepumph. Also assigning to @AgustinVallejo to ride along, so he can see how this is done.

@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 2, 2023

Almost half of the missing required tandems were due to an instrumentation bug in KeypadDialog, see phetsims/scenery-phet@ae1de72. So now we're down to 232 missing required tandems.

pixelzoom referenced this issue in phetsims/solar-system-common Nov 2, 2023
pixelzoom added a commit that referenced this issue Nov 2, 2023
@pixelzoom
Copy link
Contributor Author

Down to 82 missing required tandems.

The way I'm identifying these is to just guess based on the tandemName shown in the console. The phetioID (e.g. "mySolarSystem.requiredTandem.thumbNode") is more misleading than helpful. And the stack traces are difficult to navigate and (in most cases) not helpful.

pixelzoom added a commit that referenced this issue Nov 2, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 2, 2023

Down to 46 missing required tandems. Based on the tandem names, I'm guessing that they are related to Slider and RectangularPushButton.

@zepumph
Copy link
Member

zepumph commented Nov 2, 2023

The way I'm identifying these is to just guess based on the tandemName shown in the console. The phetioID (e.g. "mySolarSystem.requiredTandem.thumbNode") is more misleading than helpful. And the stack traces are difficult to navigate and (in most cases) not helpful.

Yeah, the tandem is quite gross, and the stack trace is manual and often hard to parse. I don't know a better way to mark these entities. We could add another mode that could debugger for each required tandem that isn't passed so you can handle the hard ones one at a time. Do you have any other ideas or work flows that you wish you could do?

@pixelzoom EDIT: I moved this discussion to general issue https://github.com/phetsims/studio/issues/313.

@pixelzoom
Copy link
Contributor Author

32 error messages were due to a single HSlider, now Tandem.OPT_OUT in the above commit.

Down to 14 missing required tandems.

pixelzoom added a commit to phetsims/scenery-phet that referenced this issue Nov 2, 2023
@pixelzoom
Copy link
Contributor Author

pixelzoom commented Nov 2, 2023

I finally located the needle in the haystack. This stack trace pointed me to enterButton = new RectangularPushButton in KeypadDialog. The other 13 stack traces were related to this, and were of no help.

"Error
    at Tandem.onMissingTandem (http://localhost:8080/chipper/dist/js/tandem/js/Tandem.js:121:26)
    at Emitter.initializePhetioObject (http://localhost:8080/chipper/dist/js/tandem/js/PhetioObject.js:136:38)
    at new PhetioObject (http://localhost:8080/chipper/dist/js/tandem/js/PhetioObject.js:122:12)
    at new PhetioDataHandler (http://localhost:8080/chipper/dist/js/tandem/js/PhetioDataHandler.js:73:5)
    at new Emitter (http://localhost:8080/chipper/dist/js/axon/js/Emitter.js:34:5)
    at new PushButtonModel (http://localhost:8080/chipper/dist/js/sun/js/buttons/PushButtonModel.js:40:25)
    at new RectangularPushButton (http://localhost:8080/chipper/dist/js/sun/js/buttons/RectangularPushButton.js:38:29)
    at new KeypadDialog (http://localhost:8080/chipper/dist/js/scenery-phet/js/keypad/KeypadDialog.js:107:25)
    at new ValuesPanel (http://localhost:8080/chipper/dist/js/my-solar-system/js/common/view/ValuesPanel.js:49:26)
    at new MySolarSystemScreenView (http://localhost:8080/chipper/dist/js/my-solar-system/js/common/view/MySolarSystemScreenView.js:114:24)"

keypadDialog.enterButton was instrumented in the above commit. We now have 0 missing required tandems, and can look at removing phet-io.validation: false from package.json.

@pixelzoom
Copy link
Contributor Author

The sim is showing no errors in Studio with phetioValidation=true, so it seems safe to remove phet-io.validation: false from package.json.

@pixelzoom
Copy link
Contributor Author

phet-io.validation: false has been removed from package.json. So validation will now be done by Studio and CT. I'll leave this open until I've verified that there are no CT problems.

@pixelzoom
Copy link
Contributor Author

CT is happy. 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

4 participants