Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Image Light Feature - GSOC 2023 #6255
Image Light Feature - GSOC 2023 #6255
Changes from 5 commits
50a0c14
dfa600c
0521694
b155bac
34e6438
916f5be
c59ae6c
8b1e19c
5287a4f
b05bca6
f6899a8
8cd5775
3c9bf12
701cbee
4c63de5
b9837dc
5546093
98af50f
8dbbff5
711c5f5
52c4d42
0121439
8e9cd8a
7cf062d
93a2428
d1c19b3
b803601
dc0ef6a
f0f3a19
02d6f00
9a0c3fc
0860bd7
0ade514
07371e8
25a711b
c04d916
9dbe8a0
5cac577
5547d7d
ff3973d
b37afe5
69008c0
d713b35
6656e90
374051b
b964403
0e7c3bc
14aed59
5d99712
4fe2890
6fec78a
94e639b
f2c0f1e
e68f9f0
6df91ec
fe5e0db
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Maybe we can rephrase this into sentences so it's a bit easier to read? Also, for each map, we should say what the keys and values are. E.g. for this one, it maps a
p5.Image
used byimageLight()
to ap5.Graphics
containing the combined light it sends to each direction.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.
Do we use this function?
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.
Nope, I actually re-wrote the function in line 22. The function N2E is the one being used for now. I think I should remove the function on line 10 and rename the function at line 22.
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.
Maybe where we use 3.14159 right now we can use the
const float PI
we defined earlier, and add more decimals to 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.
Sure, I'll clean up this file. It has some extra code as well.
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.
oh should we be using your implementation of
nTOE
from the other fragment shaders instead of this? If I recall correctly, this is the older implementation that has a bug where it only uses the 0-PI range, which you fixed in your updatednTOE
.I think using
nTOE
would also mean we can remove themap
function above since this is all that uses it.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.
Done, i'm testing after the changes. If nToE works, which it will very likely. I'll remove the
mapTextureToNormal
one.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.
done, changed the
nToE
name tomapTextureToNormal
for better understanding name.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.
Identation is off
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.
Thanks, corrected it :)
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 think this still needs to be changed to
worldNormal
(and I believe that means we can remove lines 113 and 114 here too.)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.
Done, I have commented out the lines to test it. If the tests pass then I'll completely remove this section.