phetsims / expression-exchange

"Expression Exchange" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
2 stars 2 forks source link

identify potential for reuse in Equality Explorer #101

Closed pixelzoom closed 6 years ago

pixelzoom commented 7 years ago

Expression Exchange and Equality Explorer apparently have some things in common - at least that's what the Equality Explorer design document implies, and what some people on the design team have indicated.

I was supposed to do the code review for Expression Exchange, and part of that task was going to be to work with @jbphet to identify potential reuse. But I didn't do the code review, due to the timing of vacation. So @ariel-phet suggested that we identify opportunities for reuse as a specific task. Hence this issue.

@jbphet please identify things (code, patterns,...) that might be reused in Equality Explorer. Note that this issue has a high-priority because Equality Explorer is on a tight deadline, and implementation has already begun. Any reuse that isn't identified in the next week is unlikely to be useful. If this priority doesn't fit with the schedule of other things that are on your plate, please let me know asap.

jbphet commented 7 years ago

I will spend some time on this on Monday, June 12th 2017.

jbphet commented 7 years ago

I just walked through the design doc to refresh myself, and my initial thoughts are to have a look at:

CoinTermCreatorBox, which is a box containing creator nodes CoinTermCreatorNode, which is how the user creates elements in the model

If I recall correctly, there are some cases where the items dragged from the creator box can be combined with one another. ExpressionManipulationModel might be useful for how it evaluates whether to combine model elements (evaluates overlap and chooses the one with the max) and how it does it (basically just increasing an internal count for one and removing the other). Some of the code in Expression that does placement of coin terms might be useful

I would also suggest playing with Expression Exchange a bit to see if it is doing anything that I haven't identified here that might be useful for Equality Explorer and, if you'd like to collaborate on generalizing it, or just want to walk through the associated code together to see if it'd be useful, I'd be more than happy to do that with you.

pixelzoom commented 7 years ago

Thanks @jbphet.

pixelzoom commented 7 years ago

The design document for Equality Explorer says:

When a 1 and -1 overlap, they become a 0 and fade away. We’d like to mimic the behavior seen in Expression Exchange.

@jbphet can you please point me to where this fading is implemented? And please comment on why twixt OpacityTo was not used.

jbphet commented 7 years ago

Fading is implemented using a model property, see CoinTerm.existenceStrengthProperty. The view sets the opacity based on the "existence strength" of the model element.

I originally tried doing the fade out strictly in the view,, but it turned out to be more complicated. Unfortunately I don't recall the details. I think it had something to do with having a view node continuing to exist when the model node didn't - maybe because of needing to unhook the view node's attributes from the model but keeping the view node around while it faded. I'd used the existenceStrength approach before in other sims and found it worked well, so I changed over to that approach.

pixelzoom commented 7 years ago

Thanks for the info. The "fade out" is certainly view-specific, so handling it in the model doesn't feel right. Rather than reuse what's being done in EE, I think I'll investigate a view-based approach first. I'll let you know if I discover anything that might apply to EE and other sims where you've used the existenceStrength approach.

pixelzoom commented 7 years ago

Btw... In Equality Explorer, once two inverses (ie., x and -x, 1 and -1) are summed, there is no reason to represent '0' in the model. The '0' is strictly something that is shown in the view to indicate that inverses summed to zero. So I'm unlikely to run into the "view node continuing to exist when the model node didn't" issue that @jbphet mentioned in https://github.com/phetsims/expression-exchange/issues/101#issuecomment-332594013.

pixelzoom commented 7 years ago

@jbphet A question about summing inverses (x and -x, 1 and -1) to zero in your Negatives screen. I'm trying to decide how to incorporate the concept of inverses into the "creator pattern". I have separate creators for x, -x, 1 and -1. How did you go about representing the fact that x and -x are inverses, but (for example) x and 1 are not?

pixelzoom commented 7 years ago

I asked:

@jbphet A question about summing inverses ...

@jbphet and I discussed via Zoom. To jog my memory... see typeID and totalCountProperty in CoinTerm.

pixelzoom commented 6 years ago

I believe that I've hit all of the potential areas of reuse. To recap:

• Did not reuse creator pattern, too many differences between sims. • Did not reuse method of associating inverses; having a typeID smells like inheritance, which is what I used in EqEx. • Did not reuse fade animation, since it's more appropriate to do it in the view (TWEEN) in EqEx. • Did not reuse code for fade animation, but used colors, fade time, and some empirical qualities.

Closing.