move svg position of helices out of design and into view

[dave-doty]

That was, for instance, dispatching an action such as InvertXYSet, which does not change anything about the design that would get stored in the .sc file, will not change the Dart Design object and cause a warning about changing the Design with a non-UndoableAction action.

[dave-doty]

Alternately, remove Helix SVG position from the AppState entirely, and compute it in the Helices main view component render logic. This might be less efficient, but seems like a strictly better design. (i.e., fewer possibility for bugs)

BUT, test editing a very large design (especially one with many helices, like hundreds of helices) to see if this slows things down, to have that much data computation happening in the render logic.

[UnHumbleBen]

Initially, we were thinking that we could compute the svg position at the level of DesignMainHelices. However, it turns out nearly many other components in DesignMain also depend on svg position, which they access by calling the svg_base_pos function. For example, the svg_base_pos is used for creating a new strand. Therefore, we will need to compute the svg position at the level of DesignMain instead of DesignMainHelices and all the logic relying on svg_position will need be refactor so that they now access svg_position via a newly computed map instead of from Helix objects. Judging by the number of usages of Helix.svg_position and the number of usages of functions that call Helix.svg_position (in particular, svg_base_pos), the refactor might be quite large. I found 20 places where Helix.svg_position is used and 40 places where Helix.svg_base_pos is used. This gives us a rough idea of the size of the refactor.

@dave-doty any thoughts on the refactor or other suggestions?

[dave-doty]

Hopefully we can calculate all the SVG positions in DesignMain and then pass them as props (e.g., as a Map mapping Helix idx's to SVG positions, where you only populate with the necessary helices (e.g., for DesignMainStrand, only give the helices where the strand resides). Does that seem straightforward? (Not sure if it is or not.)

[UnHumbleBen]

Hopefully we can calculate all the SVG positions in DesignMain and then pass them as props (e.g., as a Map mapping Helix idx's to SVG positions, where you only populate with the necessary helices (e.g., for DesignMainStrand, only give the helices where the strand resides). Does that seem straightforward? (Not sure if it is or not.)

This idea should work, although i think it's unnecessary to worry about which helices are actually necessary and instead simply pass in the entire map of helix idx to SVG positions without worrying about which ones are actually needed. Seems straightforward enough, just might be a lot of busy work with number of usages of svg_position. I will let you know if I run into any issues.

[UnHumbleBen]

Hopefully we can calculate all the SVG positions in DesignMain and then pass them as props (e.g., as a Map mapping Helix idx's to SVG positions, where you only populate with the necessary helices (e.g., for DesignMainStrand, only give the helices where the strand resides). Does that seem straightforward? (Not sure if it is or not.)

This idea should work, although i think it's unnecessary to worry about which helices are actually necessary and instead simply pass in the entire map of helix idx to SVG positions without worrying about which ones are actually needed. Seems straightforward enough, just might be a lot of busy work with number of usages of svg_position. I will let you know if I run into any issues.

Upon further thought, I realize that we should narrow down which helices are needed for each specific component in order to prevent unnecessary rendering from React. I'll need to look into the code some more to see if this will be straightforward.

[UnHumbleBen]

@dave-doty

Update: I have completed refactoring all usages of svg_base_pos and now working on refactoring other usages of Helix.svg_position. In doing so, I realized we have not discuss how to handle use cases where the svg position is needed in the event listeners. When svg position is needed in the render stage, this is straightforward to implement. Just simply compute all the svg positions in the DesignMain component and then pass the svg position downward to the child components, as discussed above. However, when it comes to event listeners, there is no centralized place to compute the svg positions, so each event listener that needs svg position will need to calculate it from scratch. To be concrete, I found that many listeners declared in handle_keyboard_mouse_events requires computing svg position. Here are some examples:

• strand move listener
• slice bar listener
• domain move listener

I believe that we need to call the new helices_assign_svg in each one of the listeners that depend on helix svg position. This sounds a bit wasteful, but I think it is the only way considering that the svg positions are no longer stored in state. What are your thoughts @dave-doty, does this make sense or am I overlooking something?

[dave-doty]

Hmm, this is definitely a serious issue I hadn't foreseen.

I don't think computing all SVG positions in every event listener is good. I don't want to do pre-mature optimization, but some of these are called very often, e.g., the slice bar movement.

Here's one idea.

1. These event listeners make the case that the SVG positions should be stored in the state.
2. Since they are derived fields, however, we need to take care to re-compute them when needed...
3. But we want to take advantage of Redux to avoid having to manually track when other independent fields change that the SVG positions depend on.
4. However, we can't make Helix.svg_position a memoized field because memoized fields in a Built class can only depend on other fields in the class, not on external data.

If there's some way to re-structure the AppState so that the Helix SVG positions are somehow a memoized field of some class that does contain the relevant data, then it could be automatically re-calculated by the Built library. However, I don't see how to do this since SVG position depends on so many disparate things, including UI state like invert_yz and the set of selected helices, but also the Helix objects themselves in Design.

So I'm not sure what exactly to do for now. Open to other ideas.

[UnHumbleBen]

Could we make a new memoized field AppState.helix_idx_to_svg_position_map? The AppState class has all the required arguments because it can access the invert_yz and selected_helices from ui_state and Helix objects and geometry from the design

[dave-doty]

Sure, let's give that a shot. I'm worried about the potential performance, but let's just try it and test it on the large example design.