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

Implement decay on the Chart Intro Screen #97

Closed
zepumph opened this issue Jul 10, 2023 · 9 comments
Closed

Implement decay on the Chart Intro Screen #97

zepumph opened this issue Jul 10, 2023 · 9 comments

Comments

@zepumph
Copy link
Member

zepumph commented Jul 10, 2023

Expanding from #80, this is one of the last features to implement. There will be a possibility to reuse much of the decay logic from the decay screen, but we will need to see how it should apply to the ParticleAtomView as well as the shell layers view too.

See notes from design on 11/16/22 of design doc, https://docs.google.com/document/d/12kmOdYKG1DWgMHGodMsY-rO75QS3xxHrzrjkJpqPUxQ/edit:

Keep mini animations on mini nucleons. But fade in/out nucleons on energy levels. Ex. Mini alpha particle shoots out of nucleus and in energy levels, 2 protons and 2 neutrons fade out in the same fashion (top to bottom, left to right)

@zepumph
Copy link
Member Author

zepumph commented Jul 10, 2023

The above is a start at trying to tease out the common decay code between screens. We got to a point where we are calling the screen 1 decay methods in screen 2, but it isn't working at all correctly. I am not quite sure how to change those functions to support the multiple views we have in the chart intro screen when decaying, but we will keep on working on it.

@zepumph
Copy link
Member Author

zepumph commented Jul 11, 2023

  • Toggle enabled of Decay button depending on if there is a decay?

Luisav1 added a commit that referenced this issue Jul 11, 2023
Luisav1 added a commit that referenced this issue Jul 12, 2023
Luisav1 added a commit that referenced this issue Jul 12, 2023
Luisav1 added a commit that referenced this issue Jul 17, 2023
Luisav1 added a commit that referenced this issue Jul 17, 2023
Luisav1 added a commit that referenced this issue Jul 18, 2023
@zepumph
Copy link
Member Author

zepumph commented Jul 25, 2023

Work completed above. I will review from here.

@zepumph
Copy link
Member Author

zepumph commented Jul 26, 2023

  • The electrons and positrons emitted in decay need to be smaller than the the scaled down atom in the chary screen. Right now they are the same size, because they are scaled down from the larger particles (from logic in the decay screen).

zepumph added a commit that referenced this issue Jul 26, 2023
@zepumph
Copy link
Member Author

zepumph commented Jul 26, 2023

Review:

  • This comment confuses me, isn't this already the subclass?

    public override addOutgoingParticle( particle: Particle ): void {
    // no need to add the outgoing particle because that's done in a subclass
    }

  • ff36bfd#diff-a6b7780c2f9434941e367776926b5a0320ba17f1d3ea9ffc99b6500e1dbd2a55 makes sense to me, but there are other usages of this.model.particleAtom in the BANScreenView decay logic. Can you take another look at them, for example here, to make sure they shouldn't be using this.getParticleAtom() (a function that I'm generally quite confused about).

  • There are still 4 TODOs pointing to this issue. One is about the decaying flag which I know is on hold until we get CT a bit cleaner. Let's get these taken care of.

  • Can we add documentation to any abstract function about how it should be implemented.

  • I feel like the sorting logic for finding the right particle to remove should be factored out, and maybe just called from something like this.model.particleAtom.extractParticle( particleType ). Why can't we do that for this or elsewhere?

    const protonsToRemove = _.sortBy( [ ...particleAtom.protons ], proton =>
    proton.positionProperty.value.distance( particleAtom.positionProperty.value ) )
    .slice( 0, NUMBER_OF_PROTONS_IN_ALPHA_PARTICLE );
    const neutronsToRemove = _.sortBy( [ ...particleAtom.neutrons ],
    neutron => neutron.positionProperty.value.distance( particleAtom.positionProperty.value ) )
    .slice( 0, NUMBER_OF_NEUTRONS_IN_ALPHA_PARTICLE );

  • Just an idea here, instead of EmitAlphaParticleValues, what if you subtyped ParticleAtom to store the distance and duration onto it, and then you could just return the AlphaParticleDecayParticleAtom or whatever you want to store it. In general I think it is a bit sloppy to return arrays/objects as return types because you want a function to have multiple return values. That said I have definitely done it, and with typescript it isn't really confusing or anything. Up to you!

  • Same exact question, but for BetaDecayReturnValues. There we don't have quite the same thing, so perhaps this is moot and unhelpful.

  • Would a simplification like this work?

    Subject: [PATCH] clearIncomingParticle needs to update the ParticleNucleus shell positions, https://github.com/phetsims/build-a-nucleus/issues/74
    ---
    Index: js/common/view/BANScreenView.ts
    IDEA additional info:
    Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
    <+>UTF-8
    ===================================================================
    diff --git a/js/common/view/BANScreenView.ts b/js/common/view/BANScreenView.ts
    --- a/js/common/view/BANScreenView.ts	(revision 4596d78aa47c8b83e1ef80892db8f468f053d258)
    +++ b/js/common/view/BANScreenView.ts	(date 1690496602704)
    @@ -904,19 +904,13 @@
           particleAtom.removeParticle( nucleon );
           alphaParticle.addParticle( nucleon );
           this.addOutgoingParticle( nucleon );
    +      this.findParticleView( nucleon ).inputEnabled = false;
         } );
     
         // ensure the creator nodes are visible since particles are being removed from the particleAtom
         alphaParticle.moveAllParticlesToDestination();
         this.checkIfCreatorNodesShouldBeVisible();
     
    -    alphaParticle.protons.forEach( proton => {
    -      this.findParticleView( proton ).inputEnabled = false;
    -    } );
    -    alphaParticle.neutrons.forEach( neutron => {
    -      this.findParticleView( neutron ).inputEnabled = false;
    -    } );
    -
         // animate the particle to a random destination outside the model
         const destination = this.getRandomExternalModelPosition();
         const totalDistanceAlphaParticleTravels = alphaParticle.positionProperty.value.distance( destination );
    

@zepumph
Copy link
Member Author

zepumph commented Jul 27, 2023

  • We want to make the "fade" in/out for decay in the nucleus be exactly 1 second, and not based on the particle flying away distance.

@zepumph
Copy link
Member Author

zepumph commented Jul 27, 2023

Looking really good. Thanks for all this work. Let me know if you want to pair about any of the above. Over to you.

@zepumph zepumph assigned Luisav1 and unassigned zepumph Jul 27, 2023
Luisav1 added a commit that referenced this issue Jul 31, 2023
Luisav1 added a commit that referenced this issue Jul 31, 2023
…sert to check that a beta decay particle doesn't dissapear inside visible bounds. See #97.
@Luisav1
Copy link
Contributor

Luisav1 commented Aug 1, 2023

  • For the sorting logic, an particleAtom.extractParticle( particleType ) function already exists which returns an arbitrary instance of a particle, but I did create an particleAtom.extractParticleClosestToCenter( particleType ) function.
  • This won't work for other cases, such as in the betaDecay function since that function relies on the particle closest to the center being a part of the particleAtom and particleAtom.extractParticleClosestToCenter( particleType ) removes a particle.

Luisav1 added a commit that referenced this issue Aug 2, 2023
Luisav1 added a commit that referenced this issue Aug 2, 2023
…ParticleAtom and instead use the BANScreenView function implementations. See #97.
Luisav1 added a commit that referenced this issue Aug 2, 2023
…t no longer needed in supertype emitNucleon. See #97.
@Luisav1
Copy link
Contributor

Luisav1 commented Aug 2, 2023

Thanks @zepumph for the review, all of these are now fixed in the above commits. Closing.

@Luisav1 Luisav1 closed this as completed Aug 2, 2023
Luisav1 added a commit that referenced this issue Aug 8, 2023
…emoveParticle() correctly using this.removeParticle.bind( this ). See #97.
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

2 participants