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

Settings for gradient/normal thresholds #208

Merged
merged 3 commits into from
Jul 1, 2023
Merged

Conversation

mlavik1
Copy link
Owner

@mlavik1 mlavik1 commented Jun 24, 2023

#175

Gradient visibility threshold + gradient lighting threshold

Gradient lighting threshold

This slider lets you change the minimum and maximum threshold for gradient contribution to lighting.
image

Voxels with gradient < min will be unlit. Gradient >= max will be fully shaded. Values in between will be partly shaded (for smooth transitions).

Results

image

Gradient lighting threshold

In isosurface rendering mode, you can now set a minimum gradient magnitude value. Voxels with gradient magnitude less than this will not be rendered.

image

Results

image

image

@mlavik1
Copy link
Owner Author

mlavik1 commented Jun 24, 2023

@SitronX Here's the changes I talked about! Feel free to have a look and let me know if this is useful for you, and if you have any feedback :)

Does the "Gradient lighting threshold" setting make sense to you? I had difficulty coming up with a sensible name and description..

I'll plan to start on the Job System task next :)
A bit delayed, sorry! Managed to catch a cold in peak summer 😅 And now my daughter is sick, haha.

@mlavik1 mlavik1 marked this pull request as ready for review June 24, 2023 20:27
@SitronX
Copy link
Contributor

SitronX commented Jun 26, 2023

Hello @mlavik1
The names sounds right with me, it makes sense and i also cannot think of better names :)

Can i just ask why is there this line of code: newtreshold=newtreshold*newtreshold on line 71 in VolumeRenderedObjectCustomInspector.cs. It is in iso and dvr mode. Does it have some purpose?

Also when i looked in shader, in DVR mode, it would be maybe nice if the lightning calculation could be skipped entirely if gradient magnitude is outside of interval, saving quite a bit of performance?

Otherwise it looks great, this is great idea and results are looking nice!

Managed to catch a cold in peak summer 😅 And now my daughter is sick, haha.

Ahh summer cold :D Wish you speedy recovery.

@mlavik1
Copy link
Owner Author

mlavik1 commented Jun 29, 2023

Thanks for the feedback @SitronX !

Can i just ask why is there this line of code: newtreshold=newtreshold*newtreshold on line 71 in VolumeRenderedObjectCustomInspector.cs. It is in iso and dvr mode. Does it have some purpose?

Yes, it's just using square root scaling for the slider. Without it, it would be hard to modify the lower values, which is where you'll need the most precision. I should probably write some comment 😁 Thanks for pointing it out!

Ahh summer cold :D Wish you speedy recovery.
Thanks, I'm much better now! Daughter is still ill though, hehe..

(Update: Removed the job system question I wrote here, and asking in the relevant task instead :D)

@mlavik1
Copy link
Owner Author

mlavik1 commented Jul 1, 2023

Oh, and I forgot to answer this comment:

Also when i looked in shader, in DVR mode, it would be maybe nice if the lightning calculation could be skipped entirely if gradient magnitude is outside of interval, saving quite a bit of performance?

Good point! Fetching the gradient from the gradient texture is by far the most expensive part of this, but adding an if-check to avoid lighting calculation may still inprove the performance a bit - but from my understanding, doing branching in a shader can have quite a negative impact on performance in some cases, depending on the GPU.. (maybe not so much an issue on modern GPUs?). I'll do some testing though, and see what the effect is :) I'll merge this without it for now though, and then add it if it seems to help.

Again, thanks for the feedback!

@mlavik1 mlavik1 merged commit 89d54b7 into master Jul 1, 2023
2 checks passed
@mlavik1 mlavik1 deleted the gradient-thresholds branch July 1, 2023 12:28
@SitronX
Copy link
Contributor

SitronX commented Jul 2, 2023

Hello @mlavik1

adding an if-check to avoid lighting calculation may still inprove the performance a bit - but from my understanding, doing branching in a shader can have quite a negative impact on performance in some cases, depending on the GPU

Yes you are right about that. When i look again at the lighting code, there is quite a bit of calculations, but nothing extreme. So it will probably not be worth it, because in quite a lot of cases it would do both the branching and calculations, so the benefits would nullify :D

@mlavik1
Copy link
Owner Author

mlavik1 commented Jul 2, 2023

Yes you are right about that. When i look again at the lighting code, there is quite a bit of calculations, but nothing extreme. So it will probably not be worth it, because in quite a lot of cases it would do both the branching and calculations, so the benefits would nullify :D

I think so too! I haven't checked recently, but I'm pretty sure the bottleneck will usually be texture reads (especially when using gradient texture!). So using another format, as you suggested, will likely be a much better optimisation :)

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.

2 participants