-
Notifications
You must be signed in to change notification settings - Fork 27
Make user bounding box updates more efficient #8492
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: master
Are you sure you want to change the base?
Conversation
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@MichaelBuessemeyer How would you feel about three new actions, Add/Delete/Update bbox? That way we don’t need an update action for every property. |
Yes, I agree that there are already a lot of update actions. My idea was to follow how it is done for skeleton update actions. There we also have the update action on such a detailed degree. Moreover, when thinking about the upcoming collab feature, the design doc states:
I can image the same problem with bounding boxes:
At 4. the frontend now needs to be really smart to first find out by a diff that the name of the bounding box was changed. A simple comparison with the current BBox state of User B is not possible as the diff would also reveal that the color changed (done by User B). But only the name did and only the name should be updated. This complexity could be saved in case we have an action for each field making things easier implementation wise (at least that's how I see it on first thought 🤔) Although as the bounding boxes state are saved at the time Users B last successful update was submitted to the backend, there should be a state that can be compared with to find out that actually only the name changed. But I am not sure how complicated such a code would look like compared to a "updateBBoxNameAction" and simply applying it 🤔. To me more update actions sound more tedious code-wise but should be more straight forward to implement leading to more code lines but less code complexity |
I'm also in favor of having less update action types. I think we should aim for update actions where not all properties have to be listed. For example, one could have I think, we already had an issue for this, but I cannot find it anymore. from what I remember, the above is a bit tricky, because scala cannot distinguish between |
You are probably referring to #6657 – there we didn’t consider this for update actions but it might just work there too. |
what timestamp should be put into the updateAction? the moment the first frontend action was fired or the moment when the annotation is saved? edit: this is done within update_actions.ts with the types |
my initial solution is to call the frontend updateAction with a edit: talked with @MichaelBuessemeyer, seems to be fine |
@MichaelBuessemeyer I updated the frontend code, so that new the new updateActions are called. I tested it a bit with the old backend code, and it seems like the right actions are dispatched if the bounding boxes change. Let me know if the code seems buggy in any way! |
frontend/javascripts/oxalis/model/sagas/skeletontracing_saga.ts
Outdated
Show resolved
Hide resolved
…e-efficient-bbox-update-actions
…e-efficient-bbox-update-actions
…w add update actions
Nice 👍 awesome. Thanks a lot 🙏
Yeah sorry there is a little buggy / unexpected behaviour: Moreover, I noticed that the frontend seems to send "empty" updates: The Moreover, the cherry on top would be that multiple consecutive bounding box updates to the same bounding box would be combined into a single action. Something like this is already done by the frontend before sending the update actions. See |
@MichaelBuessemeyer thanks for the reports! I will work on this now and try to fix the three problems. |
okay, I adjusted the frontend code so that
|
let me know if you notice anything else. during my testing, adding, updating and deleting bboxes seems to work fine :) |
For reference: For now blocked. See https://scm.slack.com/archives/C5AKLAV0B/p1744295336264969 |
case "updateUserBoundingBoxesInVolumeTracing" => | ||
deserialize[UpdateUserBoundingBoxesVolumeAction](jsonValue) | ||
case "updateUserBoundingBoxVisibilityInVolumeTracing" => |
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.
@MichaelBuessemeyer fyi I removed the "in" in the string -> the new old actions have the names "updateUserBoundingBoxVisibilitySkeletonAction" and "updateUserBoundingBoxVisibilityVolumeAction"
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.
Take care not to mix the scala class names vs the "name" field (string above). This is different for most actions. I think the "name" usually does not end on "Action"
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.
yes, I was talking about the "name" field. maybe we have to update the names of the actions that are introduced with this PR because they all end on "Action" 😅
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 took the freedom to change the actions names because I think it was me who might have introduced this by mistake. @MichaelBuessemeyer feel free to revert this (675ae35) if you disagree
@@ -38,11 +41,29 @@ export type CreateSegmentUpdateAction = ReturnType<typeof createSegmentVolumeAct | |||
export type UpdateSegmentUpdateAction = ReturnType<typeof updateSegmentVolumeAction>; | |||
export type DeleteSegmentUpdateAction = ReturnType<typeof deleteSegmentVolumeAction>; | |||
export type DeleteSegmentDataUpdateAction = ReturnType<typeof deleteSegmentDataVolumeAction>; | |||
type UpdateUserBoundingBoxesInSkeletonTracingUpdateAction = ReturnType< | |||
typeof updateUserBoundingBoxesInSkeletonTracing | |||
export type AddUserBoundingBoxSkeletonAction = ReturnType< |
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.
add In
This PR add more update actions for user bounding boxes. This makes it more efficient to update a single bounding box (including adding and deleting). Moreover, the version restore view can display more details on what was actually changed (e.g. color of the bounding box).
URL of deployed dev instance (used for testing):
Steps to test:
TODOs:
updateUserBoundingBoxVisibility
actions for visibility updatesupdateUserSpecificBoundingBox[Volume|Skeleton]
)Issues:
(Please delete unneeded items, merge only when none are left open)