search

phetsims / paper-land

Build and explore multimodal web interactives with pieces of paper!
https://phetsims.github.io/paper-land/
MIT License
10 stars 1 forks source link

Bug when creating from template with multiple programs #200

Closed jessegreenberg closed 6 months ago

jessegreenberg commented 6 months ago

When creating multiple programs from a single template, the order of operations is wrong. We load each program sequentially, loading dependencies before dependent components. What we need to do is load all dependencies in all programs and then load all dependent components in all programs (like how we do for loading a project).

I noticed this when trying to load a template that used a NamedDerivedProperty with dependencies in a different program.

jessegreenberg commented 6 months ago

Here is a patch for this before needing to work on something else.

```patch Subject: [PATCH] focusPanDirection -> limitPanDirection, see https://github.com/phetsims/center-and-variability/issues/564 --- Index: client/creator/model/CreatorModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/client/creator/model/CreatorModel.js b/client/creator/model/CreatorModel.js --- a/client/creator/model/CreatorModel.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f) +++ b/client/creator/model/CreatorModel.js (date 1700066512121) @@ -221,26 +221,58 @@ copyProgram( program ) { assert && assert( this.programs.includes( program ), 'program is not in this list of programs' ); - const newPosition = program.positionProperty.value.plusXY( 20, 20 ); - const newProgram = this.createProgram( newPosition ); - - const programJSON = program.save(); - newProgram.copyFromOther( programJSON, this.getUniqueCopyName.bind( this ), this.allModelComponents ); - return newProgram; + const copyJSON = { programs: [ program.save() ] }; + this.copyProgramsFromJSON( copyJSON ); } /** * Create a copy of a program from the provided JSON state. The new program is given a random number. The program and * component names are copied but will include a suffix or make them unique. - * @param programJSON + * @param json * @return {ProgramModel} */ - copyProgramFromJSON( programJSON ) { - const newPosition = phet.dot.Vector2.fromStateObject( programJSON.positionProperty ).plusXY( 20, 20 ); - const newProgram = this.createProgram( newPosition ); + copyProgramsFromJSON( json ) { + if ( json.programs ) { + debugger; + + const newModelNames = {}; + json.programs.forEach( programJSON => { + + // first create all programs and load dependency model components + const newPosition = phet.dot.Vector2.fromStateObject( programJSON.positionProperty ).plusXY( 20, 20 ); + const newProgram = this.createProgram( newPosition ); - newProgram.copyFromOther( programJSON, this.getUniqueCopyName.bind( this ), this.allModelComponents ); - return newProgram; + newProgram.copyMetadataFromOther( programJSON ); + + // This will update the programJSON and newModelNames with new names for copied components. + newProgram.copyDependencyComponentsFromOther( programJSON, this.getUniqueCopyName.bind( this ), newModelNames ); + } ); + + // then load DerivedProperty components once dependencies are in place + json.programs.forEach( programJSON => { + debugger; + const program = this.programs.find( program => program.numberProperty.value === programJSON.number ); + + // We can use this directly because this JSON has had its names updated. + program.loadDependentModelComponents( programJSON, this.allModelComponents ); + } ); + + // then load controller/view components once model components are in place + json.programs.forEach( programJSON => { + const program = this.programs.find( program => program.numberProperty.value === programJSON.number ); + + const getUniqueCopyName = this.getUniqueCopyName.bind( this ); + program.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, this.allModelComponents, newModelNames ); + program.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, this.allModelComponents, newModelNames ); + program.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, this.allModelComponents, newModelNames ); + } ); + + // then update custom code after all renames are complete + json.programs.forEach( programJSON => { + const program = this.programs.find( program => program.numberProperty.value === programJSON.number ); + program.copyCustomCodeFromOther( programJSON, this.getUniqueCopyName.bind( this ), newModelNames ); + } ); + } } /** @@ -330,9 +362,7 @@ try { const templateJSON = JSON.parse( templateJSONString ); debugger; - templateJSON.programs.forEach( programJSON => { - this.copyProgramFromJSON( programJSON ); - } ); + this.copyProgramsFromJSON( templateJSON ); } catch( error ) { this.errorOccurredEmitter.emit( error.message ); Index: client/creator/model/ProgramModel.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/client/creator/model/ProgramModel.js b/client/creator/model/ProgramModel.js --- a/client/creator/model/ProgramModel.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f) +++ b/client/creator/model/ProgramModel.js (date 1700064639261) @@ -114,20 +114,30 @@ * @param {NamedProperty[]} allComponents - all model components in all programs */ copyComponentsFromOther( programJSON, getUniqueCopyName, allComponents ) { - const newModelNames = this.modelContainer.copyModelComponentsFromOther( programJSON.modelContainer, getUniqueCopyName, allComponents ); + // const newModelNames = this.modelContainer.copyModelComponentsFromOther( programJSON.modelContainer, getUniqueCopyName, allComponents ); - this.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, allComponents, newModelNames ); - this.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, allComponents, newModelNames ); - this.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, allComponents, newModelNames ); + // this.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, allComponents, newModelNames ); + // this.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, allComponents, newModelNames ); + // this.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, allComponents, newModelNames ); - return newModelNames; + // return newModelNames; } - copyFromOther( programJSON, getUniqueCopyName, allComponents ) { - this.copyMetadataFromOther( programJSON ); - const newModelNames = this.copyComponentsFromOther( programJSON, getUniqueCopyName, allComponents ); - this.copyCustomCodeFromOther( programJSON, newModelNames ); + // copyFromOther( programJSON, getUniqueCopyName, allComponents ) { + // // this.copyMetadataFromOther( programJSON ); + // // const newModelNames = this.copyComponentsFromOther( programJSON, getUniqueCopyName, allComponents ); + // // this.copyCustomCodeFromOther( programJSON, newModelNames ); + // } + + copyDependencyComponentsFromOther( programJSON, getUniqueCopyName, newModelNames ) { + return this.modelContainer.copyDependencyModelComponentsFromOther( programJSON.modelContainer, getUniqueCopyName, newModelNames ); } + + // copyDependentComponentsFromOther( programJSON, getUniqueCopyName, allComponents, newModelNames ) { + // this.controllerContainer.copyComponentsFromOther( programJSON.controllerContainer, getUniqueCopyName, allComponents, newModelNames ); + // this.viewContainer.copyComponentsFromOther( programJSON.viewContainer, getUniqueCopyName, allComponents, newModelNames ); + // this.listenerContainer.copyComponentsFromOther( programJSON.listenerContainer, getUniqueCopyName, allComponents, newModelNames ); + // } /** * TODO: Remove any other connections with other programs. Index: client/creator/model/ProgramModelContainer.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/client/creator/model/ProgramModelContainer.js b/client/creator/model/ProgramModelContainer.js --- a/client/creator/model/ProgramModelContainer.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f) +++ b/client/creator/model/ProgramModelContainer.js (date 1700064997945) @@ -202,9 +202,56 @@ */ copyModelComponentsFromOther( otherContainerJSON, getUniqueCopyName, allComponents ) { + // // Need to map dependencies to the new copies so that we can determine whether the dependency should be + // // on a copied component or on the original. + // const nameChangeMap = {}; + // + // // Update names of components and look for dependencies + // for ( const key in otherContainerJSON ) { + // const components = otherContainerJSON[ key ]; + // + // components.forEach( component => { + // const originalName = component.name; + // + // // update the name to be unique + // component.name = getUniqueCopyName( originalName ); + // + // // store the change so that we can appropriately update dependencies + // nameChangeMap[ originalName ] = component.name; + // } ); + // } + // + // // Update dependency relationships and references in custom code. If a dependency was copied then use the new copy. + // for ( const key in otherContainerJSON ) { + // const componentObjects = otherContainerJSON[ key ]; + // + // componentObjects.forEach( componentStateObject => { + // if ( componentStateObject.dependencyNames ) { + // + // // update the dependency to use the newly copied component if it exists + // componentStateObject.dependencyNames = componentStateObject.dependencyNames.map( dependencyName => { + // return nameChangeMap[ dependencyName ] || dependencyName; + // } ); + // + // // update the derivation function to use the newly copied component if necessary + // for ( const name in nameChangeMap ) { + // const newName = nameChangeMap[ name ]; + // componentStateObject.derivation = componentStateObject.derivation.replaceAll( name, newName ); + // } + // } + // } ); + // } + + // this.loadDependencyModelComponents( otherContainerJSON ); + // this.loadDependentModelComponents( otherContainerJSON, allComponents ); + + // return nameChangeMap; + } + + copyDependencyModelComponentsFromOther( otherContainerJSON, getUniqueCopyName, nameChangeMap ) { + // Need to map dependencies to the new copies so that we can determine whether the dependency should be // on a copied component or on the original. - const nameChangeMap = {}; // Update names of components and look for dependencies for ( const key in otherContainerJSON ) { @@ -217,6 +264,9 @@ component.name = getUniqueCopyName( originalName ); // store the change so that we can appropriately update dependencies + if ( nameChangeMap[ originalName ] ) { + throw new Error( 'We have already stored a change for this name. If we do it again, references will be lost.' ); + } nameChangeMap[ originalName ] = component.name; } ); } @@ -243,7 +293,6 @@ } this.loadDependencyModelComponents( otherContainerJSON ); - this.loadDependentModelComponents( otherContainerJSON, allComponents ); return nameChangeMap; } Index: client/creator/react/SpaceSelectControls.js IDEA additional info: Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP <+>UTF-8 =================================================================== diff --git a/client/creator/react/SpaceSelectControls.js b/client/creator/react/SpaceSelectControls.js --- a/client/creator/react/SpaceSelectControls.js (revision 9e6fb99b8d6cb6959b655024a1f4508d804d9e9f) +++ b/client/creator/react/SpaceSelectControls.js (date 1700065144945) @@ -63,7 +63,7 @@ setSelectedSpaceName( 'jg-tests' ); setTimeout( () => { - setSelectedProjectName( 'projection-test' ); + setSelectedProjectName( 'derived-test' ); }, 500 ); }, 500 ); } ```
jessegreenberg commented 6 months ago

This required a brand new implementation for copying programs and templates. Since we don't have unit tests Ill have to test as much as I can manually.

jessegreenberg commented 6 months ago

I was able to test ~half way through the above list and so far its looking good. I was able to successfully create 3 programs that use a Derived component from a template (template Described Grid), which was the original motivation for the issue. @brettfiedler offered to take on the rest, thanks so much! Ill commit the changes to the dev branch.

brettfiedler commented 6 months ago

Able to create copies of programs with animation link as well as standard link components. Verified this worked for inter- and intra- program links.

brettfiedler commented 6 months ago

Custom Code Copying

  1. Tried a custom code PhET Checkbox in bf-tests (after fixing all references of paperProgramNumber -> paperNumber and paperPoints -> points) - copying worked a-okay.

  2. Tried copying both the Circle and Position custom code programs in creator-tests > custom-code-test. Using the copies in place of the originals worked fine!

brettfiedler commented 6 months ago

Okay! There's potential funk here, but it might be the details of the implementation of this program.

I can successfully recall custom code programs from a template. I did this in local spaces using the PhET Checkbox and the Circle test above. I decided to try something more complicated and made a copy of AE's camera capture projection demo.

  1. I copied the project into bf-tests, then made a global template of it ("Camera Capture and Filter").
  2. I hijacked another project (j-card) and copied in the template.

This all worked fine! Successfully used the program as it was in AE's space.

___ END OF PIECES FOR THIS ISSUE - I THINK EVERYTHING WORKS FINE____

___BEGIN NOTES ON WHAT I THINK IS A NON-ISSUE____

  1. Made duplicates of each program and tried to run the copies (not the originals). The input program worked, but the output program did not. I am assuming a reference is lost somehow? (YES, see below)
  2. I then put all 4 programs (two originals and two duplicates) into the camera and ran into this scenario. The original and copy output program is looking to the original input program for the camera feed (copy input has no effect but has its own feed). image

I think this might be due to line 172 in the Output Area_Copy where the inputBounds has not been renamed "inputBounds_copy", which makes sense since that program is external to the program that was copied and there is no anticipation that the other program was going to be copied! I think the onus here might be on the user to fix the dependencies in this instance.

// Draws the video feed onto the canvas
function drawVideo() {

    // the input bounds come from another program so we need to be graceful in how we 
    // access it (it might not be added to the camera yet)
    const inputModelBoundsProperty = phet.paperLand.getModelComponent( 'inputBounds' ); ***LINE 172***
    if ( inputModelBoundsProperty ) {
        const inputModelBounds = inputModelBoundsProperty.value;
        const inputViewBounds = phet.paperLand.utils.paperToBoardBounds( inputModelBounds, sharedData.displaySize.width, sharedData.displaySize.height );

        const outputModelBounds = phet.paperLand.getModelComponent( 'outputBounds_Copy1' ).value;
        const outputViewBounds = phet.paperLand.utils.paperToBoardBounds( outputModelBounds, sharedData.displaySize.width, sharedData.displaySize.height );
        canvasContext.clearRect(0, 0, canvasElement.width, canvasElement.height );
brettfiedler commented 6 months ago

@jessegreenberg, I think all of copying and templating works for these! Is there anything else you wanted to see or more trials?

jessegreenberg commented 6 months ago

Awesome, thanks so much for going through this! I read through https://github.com/phetsims/paper-land/issues/200#issuecomment-1814978295 and agree with what you found. OK, closing!