phetsims / number-line-distance

"Number Line: Distance" is an educational simulation in HTML5, by PhET Interactive Simulations.
GNU General Public License v3.0
0 stars 3 forks source link

Consider using points instead of point controllers in DistanceShadedNumberLineNode #14

Closed jbphet closed 3 years ago

jbphet commented 3 years ago

Related to #9 (mid-project code review).

The code in DistanceShadedNumberLineNode monitors the positions of the point controllers and creates and updates a shaded area between the points on the number line in order to indicate distance. It seems to me that it would be substantially simpler to monitor the points on the number line and update the shaded area that way instead. As it is, there is currently code at the beginning of the update method that looks like this:

        if ( !model.areBothPointControllersControllingOnNumberLine() ) {
          return;
        }

This code is run every time a point controller is dragged, regardless of whether it affects anything on the number line. This seems unnecessary and makes the code a bit confusing. Instead, I would recommend watching the points on the number line and updating the shaded area based on them. The point controllers wouldn't need to be involved at all.

@SaurabhTotey - I'd be happy to discuss on a call if you'd like any more specifics.

SaurabhTotey commented 3 years ago

I believe this was also discussed in the fall of last year, and, if I recall correctly, I started working on this change when I realized that, while using the points would be cleaner than using their controllers, I thought I needed the controllers because they have an ordering that I cannot easily check with the points.

To use the NumberLinePoints, I need to listen and track changes as the points are added and removed, which I recall thinking was not a workable solution because many parts of the code need to be able to track which point corresponds to the 'primary' point controller, and which to the 'secondary' point controller.

However, upon reviewing this once more, I realized now that I can query the point controllers to see which point controller any given NumberLinePoint belongs to: I can track the points as they come and go and find if they are associated with the primary point controller or the secondary point controller, which should allow me to introduce this change.

Taking a brief pass through, it seems like NumberLinePoint can know what its controller is, which would make this solution even easier (then I wouldn't even need to check the controllers for whether the point belongs to them, but instead just get the controller from the point), but that would require me to make the controller field of NumberLinePoint public. As far as I can tell, NumberLinePoint doesn't use its controller field, so it seems like that change should be trivial (although it does make me question why the controller is then even a field of a NumberLinePoint).

I'll continue investigating this a bit more before I start these refactors.

SaurabhTotey commented 3 years ago

I think I have come up with a suitable solution for this and #16. Since both point controllers are passed to AbstractNLDBaseModel, I made a model property that gets updated when number line points are added, moved, and removed, and the property stores the values of the number line points in a way that they can be corresponded to the controller. Now DistanceShadedNumberLine and the distance statement listen to that property which has the number line points' values. I will close this issue, but feel free to reopen if there is more to be done for this issue.