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

Fix for ssr search dist #2924

Merged
merged 3 commits into from Nov 20, 2023
Merged

Fix for ssr search dist #2924

merged 3 commits into from Nov 20, 2023

Conversation

e2002e
Copy link
Contributor

@e2002e e2002e commented Aug 27, 2023

This is a simple tweak to the ssr shader that allows to match the search distance.

In the rayCast loops on maxSteps which is an hardcoded, if the step size is too small we don't reflect far:
Capture d’écran du 2023-08-27 18-09-35

To match the searchDist we need to take the stepSize and search dist into account when setting the maxStep value:

#define maxSteps int(ceil(1.0 / ssrRayStep) * ssrSearchDist)

Capture d’écran du 2023-08-27 18-09-10

@e2002e e2002e closed this Aug 27, 2023
@e2002e e2002e reopened this Aug 28, 2023
@e2002e e2002e closed this Sep 3, 2023
@e2002e e2002e reopened this Sep 9, 2023
@e2002e e2002e mentioned this pull request Sep 12, 2023
@e2002e e2002e closed this Sep 12, 2023
@e2002e e2002e reopened this Sep 22, 2023
@MoritzBrueckner
Copy link
Collaborator

Looks good to me, but what's the reason you replaced const by #define? Defines are literally replaced in the code by the preprocessor, so instead of

for (int i = 0; i < maxSteps; i++) {

the compiler sees

for (int i = 0; i < int(ceil(1.0 / ssrRayStep) * ssrSearchDist); i++) {

Have you checked whether the compiler is able to optimize that so that the defined expression is only evaluated once? Otherwise using another solution might be faster. If const doesn't work because the value is not perceived by the compiler as being constant, you could just calculate it in Python and include the resulting value in compiled.inc like ssrRayStep and ssrSearchDist.

@e2002e
Copy link
Contributor Author

e2002e commented Oct 22, 2023

You'r right I didn't think about glsl optimizations (if there are any is this case). So here i rolled back the value type.

@e2002e
Copy link
Contributor Author

e2002e commented Oct 26, 2023

@MoritzBrueckner I realized I have missed the point about variable optimization.
Yes using a macro will recompute the values for each fragment at each frame instead of having a constant stored.
No reasons to do so.

@e2002e e2002e mentioned this pull request Nov 1, 2023
@e2002e
Copy link
Contributor Author

e2002e commented Nov 17, 2023

@luboslenco can you merge that ? It's reviewed by @MoritzBrueckner and fixes a bad bug !

@luboslenco luboslenco merged commit 0d14c70 into armory3d:main Nov 20, 2023
@MoritzBrueckner MoritzBrueckner added the Release Notes: Fixes A pull request that fixes something. Used to generate release notes. label Nov 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release Notes: Fixes A pull request that fixes something. Used to generate release notes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants