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

Small step towards VR sunflares #178

Open
wants to merge 1 commit into
base: dev
Choose a base branch
from

Conversation

JonnyOThan
Copy link
Contributor

-use the correct eye to calculate the sun's viewport position
-don't reset extinction until the right eye is done
-however something isn't working properly; the sun's viewport position doesn't seem to get updated in the shader properties for the right eye. Using Shader.SetGlobalVector instead of Material.SetVector seems to work, but that would break multi-star systems.

-use the correct eye to calculate the sun's viewport position
-don't reset extinction until the right eye is done
-however something isn't working properly; the sun's viewport position doesn't seem to get updated in the shader properties for the right eye.  Using Shader.SetGlobalVector instead of Material.SetVector seems to work, but that would break multi-star systems.
@JonnyOThan
Copy link
Contributor Author

Hey, is the sunflare shader in the OldShaders folder still representative of what scatterer is using? Cause I'm wondering if the sunViewPortPos is no longer explicitly declared by the shader as a property.

@LGhassen
Copy link
Owner

LGhassen commented Sep 4, 2022

Hey, I haven't checked this PR yet, I will check it the next time I'm modding.

For the flare shader yes it is still exactly the same. The sunViewPortPos and everything under CGInclude are variables to be passed from CPU code. I don't know what you mean exactly about property? If you mean declaring them in the properties section at the top of the shader, I never used that and don't know what it's for.

@JonnyOThan
Copy link
Contributor Author

Yeah that's exactly what I mean - the properties are the things passed in from the CPU side. For whatever reason, trying to change this one in between rendering the left & right eye doesn't seem to work.

It's super strange; seems sort of like a bug in unity...Interestingly the stock unity lens flares work perfectly in VR. Do you think it's reasonable to use those to handle the rendering? I think you could still drive all the parameters that scatterer needs for extinction etc. For now I'm just adding a cfg patch to KerbalVR to disable scatterer's sunflares; maybe I can switch to using FlareReplacer instead, but I haven't looked into it too deeply.

@LGhassen
Copy link
Owner

LGhassen commented Sep 4, 2022

It could be that this function accounts only for one eye? https://github.com/LGhassen/Scatterer/blob/dev/scatterer/Effects/SunFlare/SunFlare.cs#L136
It uses the scaled space camera. Have you tried debugging if the value changes on plugin side before being changed as a parameter?

@JonnyOThan
Copy link
Contributor Author

JonnyOThan commented Sep 7, 2022

Yes, you'll notice in the PR that I changed that to use the current camera's current active eye. I added logging and verified with the debugger that different values are being sent to the shader for each eye, but the shader only ever sees the values for the left eye (verified with RenderDoc).

And note that it DOES work correctly when I use Shader.SetGlobalVector, so either the material reference we are setting the property on isn't the right material or there's some bug in unity or I'm not understand some kind of limitation about the system.

@LGhassen
Copy link
Owner

LGhassen commented Sep 7, 2022

This is very weird. Is there any difference in behaviour between tracking station/map view and flight? Does map view work while flight is broken?

@LGhassen
Copy link
Owner

LGhassen commented Sep 7, 2022

Otherwise try to make 2 materials one for each eye and switch them?

@LGhassen
Copy link
Owner

Since we established that the issue is material related (like with the clouds), I'll just pass the local or scaled position of the sun to the shader and let the shader calculate directly the viewport position with it's current MVP matrix, that should fix it.

@LGhassen
Copy link
Owner

@JonnyOThan check my latest commit

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