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

feat(surface rendering): Add surface rendering as segmentation representation #808

Merged
merged 37 commits into from
Oct 19, 2023

Conversation

rodrigobasilio2022
Copy link
Contributor

@rodrigobasilio2022 rodrigobasilio2022 commented Oct 3, 2023

Context

Adds Surface geometry support to stack, volume and volume3D viewports.

See these examples

Changes & Results

New Geometry is added called Surface for segmentations. We also added a new tool called SurfaceIntersection to show overlay for each surface as well.

Testing

Checklist

PR

  • [] My Pull Request title is descriptive, accurate and follows the
    semantic-release format and guidelines.

Code

Public Documentation Updates

  • [] The documentation page has been updated as necessary for any public API
    additions or removals.

Tested Environment

  • [] "OS:
  • [] "Node version:
  • [] "Browser:

@sedghi
Copy link
Member

sedghi commented Oct 3, 2023

seems like we need to call reset camera clipping range AFTER the surface is added I beleive so we need to figure out when is that

https://share.cleanshot.com/rYwKdRHq

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

Much better now

I feel like we can set the planes if they were already computed. Since that closedSurfaceFilter is too slow

What I mean is if I'm slice 10 and I go to slice 11 it will be slow and that is because of the filter which I understand. Then if someone goes to 12 again it will be slow since it is the first time that we are in 12, BUT if we go back to 11, I think we should read from cache somewhere so that it will be fast.

@rodrigobasilio2022
Copy link
Contributor Author

Much better now

I feel like we can set the planes if they were already computed. Since that closedSurfaceFilter is too slow

What I mean is if I'm slice 10 and I go to slice 11 it will be slow and that is because of the filter which I understand. Then if someone goes to 12 again it will be slow since it is the first time that we are in 12, BUT if we go back to 11, I think we should read from cache somewhere so that it will be fast.

I did that. Not sure if the cache is in the right place

…/rodrigobasilio2022/feat/surface-segmentation-2
@@ -14,6 +14,7 @@ export function createContourSet(
id: contourSetData.id,
data: contourSetData.data,
color: contourSetData.color,
segmentIndex: 1,
Copy link
Member

Choose a reason for hiding this comment

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

why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To calm down the typescript error

const actorEntries = this.getActors();
actorEntries.forEach((actorEntry) => {
await actorEntries.forEach(async (actorEntry) => {
Copy link
Member

Choose a reason for hiding this comment

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

what is async here that you need an await?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because if we remove the await, the 3d volume render will not correctly call the renderer.resetCameraClippingRange();

Copy link
Member

Choose a reason for hiding this comment

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

But which part of the the inner functions is asynchronous? Do you know?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I understand what you are saying... I really dont know.... Any of these functionas appears to be the one. I remember if I removed this await if will not render correctly the volumeViewport3D. Maybe you can do this test again...

Comment on lines +2038 to +2044
const oldActors = this.getActors();
if (oldActors.length && oldActors[0].uid === this.id) {
oldActors[0].actor = actor;
} else {
oldActors.unshift({ uid: this.id, actor });
}
this.setActors(oldActors);
Copy link
Member

Choose a reason for hiding this comment

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

can you explain what is happening here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If one adds an actor in the stackViewport this part of code will remove it from the actors list, because old code creates a list with only the ImageSlice actor and sets it as the actors list. So what I am doing here is checking if the imageslice actor is in the actor list. If not adding as the first item of the list

Copy link
Member

Choose a reason for hiding this comment

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

I'm not following unfortunately.

So this says that if the uid of the actor is the same as the viewportId (what does that even mean), then replace the actor at the first index? otherwise throw it out?

Why?

const polyData = actorEntry.clippingFilter.getOutputData();
let worldPointsSet = getPolyDataPoints(polyData);
if (worldPointsSet) {
worldPointsSet = removeExtraPoints(viewport, worldPointsSet);
Copy link
Member

Choose a reason for hiding this comment

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

I commented this line out (to not remove extra points) and everything worked fine. Do you have a reason to include this?

@sedghi sedghi changed the base branch from feat/surface-segmentation-2 to main October 19, 2023 01:23
@netlify
Copy link

netlify bot commented Oct 19, 2023

Deploy Preview for cornerstone-3d-docs ready!

Name Link
🔨 Latest commit 62f3e44
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-3d-docs/deploys/65309439de731c0008c8c27b
😎 Deploy Preview https://deploy-preview-808--cornerstone-3d-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link
Member

@sedghi sedghi left a comment

Choose a reason for hiding this comment

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

This is great work, thanks

@sedghi sedghi changed the title Add surface clipping via filter feat(surface rendering): Add surface rendering as segmentation representation Oct 19, 2023
@sedghi sedghi merged commit f48d729 into cornerstonejs:main Oct 19, 2023
9 checks passed
@salimkanoun
Copy link
Contributor

Wow guys this is so impressive !

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

Successfully merging this pull request may close these issues.

3 participants