-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
medical volumetric rendering example and supporting files #764
Conversation
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.
Please address comments.
ProjectSettings/ProjectVersion.txt
Outdated
@@ -1 +1 @@ | |||
m_EditorVersion: 5.6.2f1 | |||
m_EditorVersion: 5.6.0f3 |
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.
Nit: Please revert this. Project is currently on 5.6.2f1.
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.
Is there some way for me (without write access) to just accept your proposed changes?
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.
You don't have write access to the branch you're trying to merge?
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 have write access to repo I forked from this, but the changes requested from here don't show up there I guess?
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.
Couldn't figure out an easy automated way to accept them so I just did it manually - seems crazy that it's not an option.
@@ -76,7 +76,6 @@ PlayerSettings: | |||
forceSingleInstance: 0 |
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.
Project settings should also be reverted.
@@ -4,4 +4,7 @@ | |||
EditorBuildSettings: | |||
m_ObjectHideFlags: 0 | |||
serializedVersion: 2 | |||
m_Scenes: [] | |||
m_Scenes: |
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.
Editor build settings should also be reverted.
Awesome work! Would be nice to get those movement controls tweaked and do some further investigations on improving the framerate. Seems like there's some sort of acceleration being applied in the movement; maybe caused by lower framerate. As an example I would expect something performant for good future foundation. |
What frame rate are you seeing, and how close to the volume are you? It's possible to improve perf further but it is extremely lean as is, the only major improvements would come from using half precision. Past that it's a reduction in quality. The movement is based on the axial input model and is unfiltered with no rails. |
I'm seeing around 20 fps while looking at the volume on the HoloLens. Here's the image: https://pasteboard.co/GAlEtMx.png |
The movement feeling like it is accelerating is due to it using navigation. I'm switching it to manipulation and will forgo using rails. |
Addressed all feedback I believe? @Ziugy - https://github.com/Microsoft/HoloToolkit-Unity/tree/547ec954ba8259bbc0d65107bbf45dcc81526f88 That's the repo snapshotted to the lastest update with the interactions changed and the volume sized down. |
Thanks! Was looking for a screenshot of what you were seeing so I could see how close you are, but no worries - the brain is half scale now as it was about 3' along each axis before which is what was causing the perf problems. It's probably about right now - half a meter/1.5', but the brain doesn't completely fill the volume so yeah relatively close. Can you let me know if you have any issues with the latest? |
Ping! Anything I can do to help get eyes on this? |
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.
Please add a readme in your examples folder describing the what and how to use and run your scene. Sorry if I missed it in 600 files :)
|
||
namespace HoloToolkit.Unity | ||
{ | ||
public static class FileSystemHelper |
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.
Please add class descriptions for your new classes and update the README in the Utilities folder.
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.
Class headers? Sure thing.
namespace HoloToolkit.Unity | ||
{ | ||
[CustomPropertyDrawer(typeof(SaveLocalFileAttribute))] | ||
public class SaveLocalFileEditor : PropertyDrawer |
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.
Without some descriptive text, it's getting hard to determine whether these classes truly belong in Utilities or your examples folder.
…for debugging, add a readme.md
…alized version appropriate to this version of unity
Very grateful for the excellent feedback! Also: ping! :D |
@NeerajW all good here now? |
medical volumetric rendering example and supporting files
Example of volumetric rendering specifically focusing on the medical scenario.