Skip to content
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

Bbox #699

Merged
merged 4 commits into from
May 31, 2023
Merged

Bbox #699

merged 4 commits into from
May 31, 2023

Conversation

bnmajor
Copy link
Collaborator

@bnmajor bnmajor commented May 29, 2023

This PR adds a button that allows the user to toggle on/off a label image bounding box. This will be useful when trying to visualize the actual bounds of a label image when it is not rendered as expected. This currently only adds this feature for the label image. This can be changed if the feature should be available for all images (moving, fixed, label, etc). This now supports displaying the bounding box for any image layer.

label_bbox

Currently a draft - Relies on InsightSoftwareConsortium/itk-viewer-icons#5.

Should help with InsightSoftwareConsortium/itkwidgets#662 and InsightSoftwareConsortium/itkwidgets#663.

@bnmajor bnmajor requested review from PaulHax and thewtex May 29, 2023 15:27
@@ -7,6 +7,9 @@ class LayersMachineContext {

// A { name, data } object, queued for creation of the data's actor
lastAddedData = null

// Draw bounding boxes around image and label (if loaded)
labelBBoxEnabled = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this state be associated with the actual image, not in this image aggregating parent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is now a map of each layer and its bounding box visibility status. That makes sense though. Maybe each layer's status should live with its LayerActorContext instead of at the top level here?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Or the ImageActorContext, i'm not sure.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved it to the LayerActorContext for now as each layer can simply have a boolean bbox key associated with it. Moving it to the ImageActorContext would be just as simple, we'd just need an imageBBox and labelBBox (or something along those lines) associated with each image actor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most excellent.

In the future, it will also be useful to show the bounding box for point sets, etc.

@@ -148,6 +148,11 @@ function createImagesRenderingMachine(options, context) {
to: (c, e) => `imageRenderingActor-${e.data}`,
}),
},
TOGGLE_LABEL_BBOX: {
actions: send((_, e) => e, {
to: c => `imageRenderingActor-${c.images.selectedName}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this event include the name of the image in the payload like the the other events? So the UI layer pulls selectedName and puts it in data: { name: selectedName }. Then can toss the event name on the makeTransitions forwardToNamedActor block below. Things are inconsistent, but better every day.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thanks 👍

@@ -702,6 +704,16 @@ function ItkVtkViewProxy(publicAPI, model) {
model.axesCircleRadius = 4
model.axesTextOffset = 14

model.enableLabelBBox = false
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to avoid duplication of state and pull this boolean from the context?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am using the model's enabled flag to guard against unnecessary render calls by comparing it to the context state.

Copy link
Collaborator

@PaulHax PaulHax May 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could compare the visibility boolean from the actor. Or just call actor.setVisiblity and if vtk.js is working right, it will do the guard check and won't render if nothing changed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaned up 👍

const actorContext = context.images.actorContext.get(imageName)
if (actorContext?.labelImageName === multiscaleLabelImage.name) {
actorContext.labelImage = multiscaleLabelImage
service.send({ type: 'LABEL_IMAGE_ASSIGNED', data: imageName })
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@bnmajor bnmajor force-pushed the bbox branch 2 times, most recently from 9fd6526 to 2196565 Compare May 29, 2023 18:46
@thewtex
Copy link
Member

thewtex commented May 30, 2023

Awesome!!

Provide a toggle for drawing the bounds of the label image. Useful for understanding where a label
image may be in space, especially if direction or origin are incorrect.
…reset

When a label image was set the old label image was replaced but a new layer entry was still being
created. This implied all label images were available but that is not the case. If the label image
is changed the label image layer is simply updated now.
@bnmajor bnmajor marked this pull request as ready for review May 31, 2023 16:39
@bnmajor
Copy link
Collaborator Author

bnmajor commented May 31, 2023

@PaulHax @thewtex This should be ready for review now, icon has been added.

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lovely! 😍

@thewtex thewtex merged commit 3a14c39 into Kitware:master May 31, 2023
@bnmajor bnmajor deleted the bbox branch May 31, 2023 17:01
@github-actions
Copy link

🎉 This PR is included in version 14.42.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants