-
Notifications
You must be signed in to change notification settings - Fork 326
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
feat(surface rendering): Add surface rendering as segmentation representation #808
Conversation
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 |
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.
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, |
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.
why?
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.
To calm down the typescript error
const actorEntries = this.getActors(); | ||
actorEntries.forEach((actorEntry) => { | ||
await actorEntries.forEach(async (actorEntry) => { |
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.
what is async here that you need an await?
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.
Because if we remove the await, the 3d volume render will not correctly call the renderer.resetCameraClippingRange();
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.
But which part of the the inner functions is asynchronous? Do you know?
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 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...
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); |
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.
can you explain what is happening here?
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.
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
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 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); |
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 commented this line out (to not remove extra points) and everything worked fine. Do you have a reason to include this?
✅ Deploy Preview for cornerstone-3d-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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 is great work, thanks
Wow guys this is so impressive ! |
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
semantic-release format and guidelines.
Code
[] My code has been well-documented (function documentation, inline comments,
etc.)
[] I have run the
yarn build:update-api
to update the API documentation, and havecommitted the changes to this PR. (Read more here https://www.cornerstonejs.org/docs/contribute/update-api)
Public Documentation Updates
additions or removals.
Tested Environment