Physically based camera #4585
Merged
Physically based camera #4585
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mvaligursky
reviewed
Aug 23, 2022
kungfooman
reviewed
Aug 25, 2022
slimbuck
reviewed
Aug 25, 2022
slimbuck
reviewed
Sep 1, 2022
Added flag to enable/disable physically based camera.
Also added getters, setters and copying of physically based camera parameters.
GSterbrant
force-pushed
the
gsterbrant_pb_camera
branch
from
82f00ac
to
3cb2bdb
Compare
willeastcott
reviewed
Sep 1, 2022
willeastcott
reviewed
Sep 1, 2022
Co-authored-by: Will Eastcott <will@playcanvas.com>
slimbuck
reviewed
Sep 2, 2022
slimbuck
approved these changes
Sep 5, 2022
mvaligursky
reviewed
Sep 5, 2022
| } | ||
|
|
||
| /** | ||
| * Set camera shutter speed in seconds, the default value is 1s. Longer shutter means more exposure. |
There was a problem hiding this comment.
I'm really not sure about 1s as a default here. What are we going to do in the future when we perhaps add a motion blur? I think the default value should be something that would work reasonably well for future purposes.
There was a problem hiding this comment.
Addressing that here:
mvaligursky
reviewed
Sep 5, 2022
Comment on lines
+322
to
+344
| set aperture(newValue) { | ||
| this._aperture = newValue; | ||
| } | ||
|
|
||
| get aperture() { | ||
| return this._aperture; | ||
| } | ||
|
|
||
| set sensitivity(newValue) { | ||
| this._sensitivity = newValue; | ||
| } | ||
|
|
||
| get sensitivity() { | ||
| return this._sensitivity; | ||
| } | ||
|
|
||
| set shutter(newValue) { | ||
| this._shutter = newValue; | ||
| } | ||
|
|
||
| get shutter() { | ||
| return this._shutter; | ||
| } |
There was a problem hiding this comment.
Currently these functions do nothing other than se / return the value. Personally, I'd remove them, and just rename the private variables to not have underscores in them.
There was a problem hiding this comment.
This was just to have it be consistent with the other parameters, like clearDepth and clearDepthBuffer, etc.
mvaligursky
reviewed
Sep 5, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Initial PR for physical light units #3252 by enabling the use of physically based camera properties. The default properties (f/1.0, ISO 100, 1s) could have an effect on the overall lighting in the scene, so these are behind a switch.