DM-54928: Use unique mask planes for each frame#55
Conversation
jaladh-singhal
left a comment
jaladh-singhal
left a comment
There was a problem hiding this comment.
Looks good - code is very readable.
My comments are just suggestions (not blockers) - it's ok to merge even without them.
| return firefly_client.__version__ | ||
|
|
||
|
|
||
| def _scoped_mask_id(frame, plane_name): |
There was a problem hiding this comment.
This can be moved inside DisplayImpl and defined as @staticmethod - to keep it encapsulated.
| # Legacy entries may be bare plane names; treat them as belonging | ||
| # to the current frame to preserve the old behavior in that case. | ||
| if isinstance(entry, tuple): | ||
| f, name = entry | ||
| else: | ||
| f, name = frame, entry |
There was a problem hiding this comment.
I wonder if we really need to preserve the legacy behavior - any instance of this updated class will have _maskIds in tuple format. Did you encounter any case where they weren't?
There was a problem hiding this comment.
No, I can't think of any case where the old behavior would be desirable.
| if isinstance(entry, tuple): | ||
| f, name = entry | ||
| if f == frame: | ||
| names.add(name) | ||
| else: | ||
| names.add(entry) |
There was a problem hiding this comment.
same for this fallback. It's not bad to have it it just might be dead code.
There was a problem hiding this comment.
I'll remove both.
isullivan
force-pushed
the
tickets/DM-54928
branch
from
388dfb6 to
4191c8e
Compare
2 participants
No description provided.