-
Notifications
You must be signed in to change notification settings - Fork 187
fix(app): Always render labware centered over their underlying "slots" #20285
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: edge
Are you sure you want to change the base?
Conversation
The file under test got renamed a while ago, but it looks like this test file got forgotten.
| labwareChildren: ( | ||
| <> | ||
| {matchingLidDef != null ? ( | ||
| <LabwareRender | ||
| definition={matchingLidDef} | ||
| positioningMode="passThrough" | ||
| /> | ||
| <CenterLabwareInSlot definition={matchingLidDef}> | ||
| <LabwareRender | ||
| definition={matchingLidDef} | ||
| positioningMode="passThrough" | ||
| /> | ||
| </CenterLabwareInSlot> | ||
| ) : null} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change is basically the gist of this PR.
For context, BaseDeck will render labwareChildren at the origin of a deck slot. So, this code was rendering matchingLidDef at the origin of a deck slot.
Historically, for most labware, this has happened to work because it placed the bottom-left of the labware (the origin of the labware) at the bottom-left of the slot (the origin of the slot). However, it does not work for:
- Non-standard-sized labware like lids, because then the labware is visually off-center. Sometimes
cornerOffsetFromSlotcompensated for this, but it's dubious whether that should be specified for things like lids, and as a matter of practicality, it so far hasn't been specified for lids. - Labware schema 3, since the origin of the labware is not its bottom-left corner.
So the solution is to always insert this intermediate CenterLabwareInSlot transformation, which understands what's being placed into what, and explicitly takes charge of aligning the child to the middle of the parent. There are different helpers for different circumstances.
BaseDeck's labwareChildren prop is only one pattern that needed to be fixed here. Codebase-wide, we need to stay vigilant against other patterns that place labware "at the slot origin." This is almost always subtle and implicit, unfortunately, which is one of the reasons I've been so gung-ho about documentation (like how labwareChildren documents the SVG origin) and deduplication (like how BaseDeck also has a nestedLabwareDefsBottomToTop prop that takes the responsibility of doing this kind of thing away from the caller).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No new content in here, just stuff moved from labwareSchemaShims.test.ts.
| const clampedLabwareOffsetX = | ||
| Math.abs(nestedLabwareOffsetX) > LABWARE_OFFSET_DISPLAY_THRESHOLD | ||
| ? nestedLabwareOffsetX | ||
| : 0 | ||
| const clampedLabwareOffsetY = | ||
| Math.abs(nestedLabwareOffsetY) > LABWARE_OFFSET_DISPLAY_THRESHOLD | ||
| ? nestedLabwareOffsetY |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This clamping code looks like it was copied from how <Module> used to work. I removed that behavior from <Module> in #18890 basically as a simplification, after clearing it with the design team. I guess I missed that it was added here too.
We're in a better position to add it back now if we want to, but if we do, it would probably belong in <AlignToModuleChildSlot>.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contents moved to positionMath.test.ts. labwareSchemaShims was positionMath's old name and I guess this got left behind somehow after it was renamed.
sfoster1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm extremely pro all of this i think it will make our lives so much better
| <CenterLabwareInSlot definition={matchingLidDef}> | ||
| <LabwareRender | ||
| definition={matchingLidDef} | ||
| positioningMode="passThrough" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need positioning modes anymore? what do they do at this point?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We do, but only for now.
<LabwareRender> and <Module> had some intrinsic transforms that made the components difficult to compose. "passThrough" turns those transforms off so another component can do them instead; "offsetInSlot" and "offsetToSlot" preserve the old behavior. Soon (🔜) everything will use "passThrough" and then we can delete the old behavior, but we haven't quite gotten there yet. Just a matter of going through each call site and understanding what it's trying to do.
Overview
This changes the desktop app and ODD so that whenever they render a labware, they center it in the underlying slot. This is partially a shortcut to support labware schema 3, and partially a fix to avoid small labware like lids and adapters being rendered in the wrong place.
Closes EXEC-2093, EXEC-1784, RQA-4775, and RQA-4854. See EXEC-2093 for a more detailed rationale. Does not fix related bugs in Protocol Designer.
Test Plan and Hands on Testing
Here's a protocol that loads some labware in some "interesting" places:
Before this PR (take a look at slots A4 and D1):
After this PR:
Changelog
BaseDeckto do deck map rendering.Review requests
BaseDeck's API somehow?Next steps
computeLabwareOrigin().) These are not touched here.computeLabwareOrigin()needs to change to center labware instead of attempting physical accuracy.Risk assessment
High. We have a lot of deck maps and they all work a little differently, so it's difficult to test this exhaustively.