-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Shadow Map Start #3551
Shadow Map Start #3551
Conversation
How do you open a pull request like this without a screen shot 😄? |
Looks good to me so far. I don't have any comments yet. I will about how this fits into the engine when it gets to that point. @pjcozzi Do you want to do a review? |
Yes, will check this out tonight. |
setView(this, camera.viewMatrix); | ||
setInverseView(this, camera.inverseViewMatrix); | ||
setCamera(this, camera); | ||
this.updateCamera(camera); |
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 haven't looked at the rest of the code yet, but does it make sense to not call this here and require the user to call it, e.g.,
renderFrame
uniformState.update()
foreach camera
uniformState.updateCamera()
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.
Removing this.updateCamera(camera);
breaks a lot of tests, so I'm leaving it in.
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.
Add a TODO to your roadmap since those tests should probably call update
and updateCamera
(or we can make a tests-specific helper that calls both). I hate to make the render workflow more complicated so we don't have to change tests.
@@ -148,6 +148,10 @@ define([ | |||
this._resolutionScale = 1.0; | |||
|
|||
this._fogDensity = undefined; | |||
|
|||
// TODO : expose these differently |
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 it would be fine to just have a ShadowMap
reference here, and then assign to it each frame when calling UniformState.update
(pass it as a separate parameter to start, I guess).
Great start, @lilleyse! That's all my comments. |
Updated. |
110545b
to
417802b
Compare
417802b
to
517b58c
Compare
Good start! |
For #2594
Here is basic shadow mapping for glTF models, with some debug helpers.The major areas to work on next are fixing aliasing, getting shadow commands based on the light frustum, cascades/automatic light frustum fitting, support for terrain and primitives, and otherwise addressing the TODO's I left in the code.