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
Metadata picking preparation #12075
Metadata picking preparation #12075
Changes from 45 commits
656c638
247102f
e561dec
cc1b826
a28f62d
122c4e8
a44ac07
e0c57f6
dc7552c
609e277
927d01b
461821b
261dbba
3817389
a183fd4
7848f5d
66b3252
016a1a8
d575317
4d0b722
2f29019
a1989c0
88500ec
fe4c2bd
f8bac8f
8a24da2
ebddfe1
c0eaf7b
e732550
775113d
2056da0
0e6b7f6
45727e8
2a948cf
ddd5fb8
4347a23
d813dba
0ad2bc5
aa75922
9c3c081
c854235
384b2fa
3c1b497
a005521
30e80b1
9007e85
2a92b94
8b02ec4
7735111
d816418
7d34d0d
8af87b2
6054373
e1e2ae6
6f7f162
651a2b2
a7a0ece
0e47cfb
21129e2
777183f
942a178
efe3510
35bdc13
93b994a
17bc38b
048771f
4f99d1b
7300ae1
bd84faa
eb406b5
59b4787
2ee6366
280e8d9
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.
This would be easier to read with some types in the doc. I'm assuming
shaderProgram
is aShaderProgram
? Some confirmation would help me read it more confidently & quickly.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.
Would a
@see getPickShaderProgram
be enough here? 😁Seriously: I'm a HUGE fan of the "Boy Scout Rule" of programming:
Derived from the rule "Leave the campground cleaner that you found it". Now one could argue in how far this applies when boy scouts visit something that isn't a "campground" to begin with.
However, I added the information that the
shaderProgram
is aShaderProgram
. (This should be self-evident, but isn't (hint: adrawCommand
is not always aDrawCommand
)). I'd like to explain more, but explaining requires a depth of understanding that I won't claim to have.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.
What if a refactor of
Scene
makes these details obsolete? Would it be safer to be more general here?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'd love these details to become obsolete 🙂 One could accuse me of overshooting with this comment. But ... to be honest: I did spend a considerable amount of time with reading code that I should not have to read. Figuring out what
scene3DOnly
is (beyond doing a full-text search and reading, reading, reading) is not so much fun. There's alight
in theFrameState
. Is this even used anywhere? (I don't think so...). So ... we could open an issue to just clean it up and add documentation, but that would hardly be considered something actionable.If you insist, I remove the comments (or replace them with
/** @private */
😶 )