Conversation
annehaley
force-pushed
the
constraints-and-landmarks
branch
from
695a3c9 to
810ca32
Compare
…ndle subjects with different centers
…angesMade watcher
…onflicts (like with landmarks.csv and constraints.json)
…ose constraints cannot be saved.
…ay serializes as object instead of list.
…n the layer is enabled (even if no shapes are selected for rendering)
…ns before fetching is complete
JakeWags
left a comment
JakeWags
left a comment
There was a problem hiding this comment.
Approved, with a few questions. None of the questions are critical and otherwise all looks great. No interaction bugs I can find that haven't already been exposed, fixed, or both.
| and locations[0] is not None | ||
| and len(locations[0]) == 3 |
There was a problem hiding this comment.
Could you clarify why these are needed?
There was a problem hiding this comment.
This if statement is intended to ensure that incoming location data is valid and prevent any server error in the case of invalid format. The list of landmark locations must be a list with at least one element, and each element should be an [x, y, z] coordinate.
| constraints_object, created = models.Constraints.objects.get_or_create( | ||
| project=project, subject=target_subject, anatomy_type=anatomy_type | ||
| ) | ||
| file_name = 'constraints.json' |
There was a problem hiding this comment.
Is there a reason constraints is saved as json and landmarks as csv?
There was a problem hiding this comment.
This is how they are stored with Studio, and I maintained that design to ensure compatibility.
| path_split = path.split('/') | ||
| file_item.download( | ||
| Path(folder, *path_split[:-1]), | ||
| file_name=path_split[-1], | ||
| ) |
There was a problem hiding this comment.
To confirm: this is the file upload changes we've discussed?
There was a problem hiding this comment.
Yes, this change was done with the changes to swcc/swcc/models/file.py to ensure we download files with the expected file names from the project file (instead of the name of the internally-stored file).
This became a problem for projects with a custom file name for constraints or landmarks. After editing the data in Cloud, the file name is saved internally as "constraints.json" or "landmarks.csv". This internal name is fine, but when downloading, we needed to save these files with the expected names. Especially in cases with multiple domains, where more than one constraints file and/or more than one landmarks file can exist. If put in the same location with the default file name, one would overwrite the other.
| for (let p = 0; p < locationData.length; p+=3) { | ||
| const location = locationData.slice(p, p+3) as number[] |
There was a problem hiding this comment.
What is the reasoning behind p+=3 and p+3? Can the 3 value ever change or is that a constant for landmarks? I believe this is the 2nd place I've seen that.
There was a problem hiding this comment.
The value for pointData.getPoints().getData() is actually a flattened array of points, like this: [x1, y1, z1, x2, y2, z2...] In order to iterate over the points in this list, we use a value p which increases by 3 on each iteration and we get a slice of the array from p to p+3. We can do this because we know each point must have three components.
web/shapeworks/src/views/Main.vue
Outdated
| } | ||
|
|
||
| const debouncedRefreshRender = _.debounce(refreshRender, 300) | ||
| const debouncedRefreshRender = _.debounce(refreshRender, 1000) |
There was a problem hiding this comment.
Does this need to be extended for a particular reason?
There was a problem hiding this comment.
Thank you for pointing this out. I had originally done this to reduce redundant update calls before I had implemented the patchwise widget implementation. The render was getting refreshed too often as a user moved around the widgets. Now that we have patchwise updates, I can bring this number back down. I changed it back in 45ec6bc.
2 participants
This PR adds two new features to the "Info" tab: landmark editing and constraint editing. Resolves #358.
shapeworks_editing_landmarks.mp4
shapeworks_constraints.mp4