swap position x z coordinate interpretation

[dave-doty]

Undo what was done to close issue #307.

What I didn't realize when that was implemented is that it switched us from a right-handed coordinate system to a left-handed coordinate system. We want to go back to right-handed to make it simpler to export to 3D tools such as oxDNA.

Let's ensure backward compatibility here by bumping the minor version number once this is fixed, checking the version on import from a .sc file, and swapping the x-z coordinates on prior versions.

Related to Python package issue 144.

[UnHumbleBen]

@dave-doty Is this the correct way to access and write to the position field?

      if (position_x_z_should_swap && grid_is_none) {
        // prior to version 0.13.0, x and z had the opposite role
        num swap = helix_builder.position_.x;
        helix_builder.position_.x = helix_builder.position_.z;
        helix_builder.position_.z = swap;
      }

I'm running into weird issues where the fields are still null after I set them.

[dave-doty]

Should be. Make sure the builder is being built and re-stored wherever it needs. (I'm not sure which line of code this is.)

[UnHumbleBen]

@dave-doty I investigated some more and it turns out the issue has to do with the grid_is_none field in particular to dealing with helix groups. The grid_is_none field is a global variable variable, but in designs that use helix groups, there is no global grid. Is there a way to know if a helix_builder is using None grid?

[dave-doty]

It's probably a mistake to have a global grid_is_none variable. The grid should be coming from the helix group(s) (and can be different for different groups). This might be a mistaken holdover from the previous setup where there was only one grid.

[UnHumbleBen]

@dave-doty Seems like grid_is_none is always used with !using_groups, so I was thinking:

      if (position_x_z_should_swap && (grid_is_none && !grid_is_none || helix_builder_is_in_none_grid)) {
        // prior to version 0.13.0, x and z had the opposite role
        num swap = helix_builder.position_.x;
        helix_builder.position_.x = helix_builder.position_.z;
        helix_builder.position_.z = swap;
      }

Any suggestions on getting the helix_builder_is_in_none_grid variable?
Do you see any issue with using helix_builder.grid == Grid.none?

[dave-doty]

I don't know, I'd have to get back into that code and inspect to see.

If Helix has a grid field that's not nullable, then perhaps it is always set to be the appropriate grid from the helix's group, then you could that. I'd check the reducers that modify helices when the grid changes to see.

[UnHumbleBen]

I think helix_builder.grid == Grid.none should work.

It is not nullable:

scadnano/lib/src/state/helix.dart

Line 67 in 36d27dd

Grid grid,

and the GridChange action reducer sets grid for all helix builders:

scadnano/lib/src/reducers/helices_reducer.dart

Line 461 in 36d27dd

helix_builder.grid = action.grid;

Don't think any other actions touch the Grid

[UnHumbleBen]

@dave-doty Thinking of reusing this test:

scadnano/test/other_unit_test.dart

Lines 140 to 187 in 36d27dd

	test('read_old_version_position_x_z_swapped', () {
	String no_grid_two_helices_json = r"""
	{
	"version": "0.8.0",
	"grid": "none",
	"helices": [
	{
	"position": {"x": 10, "y": 60, "z": 30}
	},
	{
	"position": {"x": 20, "y": 80, "z": 50}
	}
	],
	"strands": [
	{
	"domains": [
	{"helix": 0, "forward": true , "start": 0, "end": 16}
	]
	},
	{
	"domains": [
	{"helix": 0, "forward": false , "start": 0, "end": 16}
	]
	},
	{
	"domains": [
	{"helix": 1, "forward": true , "start": 0, "end": 16}
	]
	},
	{
	"domains": [
	{"helix": 1, "forward": false , "start": 0, "end": 16}
	]
	}
	]
	}
	""";
	Design design = Design.from_json(jsonDecode(no_grid_two_helices_json), false);
	// ensure x and z are swapped after reading in
	//TODO: test for swapping x and z positions in versions < 0.9.0 temporarily disabled until
	// codenano/scadnano versions are aligned
	// expect(design.helices[0].position3d().x, 30);
	// expect(design.helices[0].position3d().y, 60);
	// expect(design.helices[0].position3d().z, 10);
	// expect(design.helices[1].position3d().x, 50);
	// expect(design.helices[1].position3d().y, 80);
	// expect(design.helices[1].position3d().z, 20);
	});

Planning to uncomment the expect statements.
Is the TODO comment relevant anymore? Thinking of removing it.

[UnHumbleBen]

Another thing to fix is to change the labels and variables associated with #308 (inverting y axis) specifically the part inverting of the z-axis would instead now be inverrting the x-axis