Added Shadows to ModelInstanceCollection #4341
Conversation
This is a possible solution for Issue CesiumGS#4340.
|
Thanks for the pull request! Can you send over a CLA so we can merge this? You can find instructions in CONTRIBUTING.md |
| @@ -72,6 +72,7 @@ define([ | |||
| * @param {Boolean} [options.asynchronous=true] Determines if model WebGL resource creation will be spread out over several frames or block until completion once all glTF files are loaded. | |||
| * @param {Boolean} [options.debugShowBoundingVolume=false] For debugging only. Draws the bounding sphere for the collection. | |||
| * @param {Boolean} [options.debugWireframe=false] For debugging only. Draws the instances in wireframe. | |||
| * @param {ShadowMode} [options.shadows] Determines the shadowmode to be used for the Models. | |||
There was a problem hiding this comment.
For organization purposes can you place this right after the options.asynchronous line. Also consider rewording the description to match Model.js, but change 'model' to 'collection'.
* @param {ShadowMode} [options.shadows=ShadowMode.ENABLED] Determines whether the model casts or receives shadows from each light source.| @@ -131,6 +132,7 @@ define([ | |||
| this._gltf = options.gltf; | |||
| this._basePath = options.basePath; | |||
| this._asynchronous = options.asynchronous; | |||
| this._shadows = option.shadows; | |||
There was a problem hiding this comment.
It would also be nice to make shadows settable whenever. Add a getter/setter shadows property to the defineProperties section that sets _model.shadows.
| @@ -131,6 +132,7 @@ define([ | |||
| this._gltf = options.gltf; | |||
| this._basePath = options.basePath; | |||
| this._asynchronous = options.asynchronous; | |||
| this._shadows = option.shadows; | |||
There was a problem hiding this comment.
This should be options.shadows.
Changed a typo, changed position of some lines to adhere to consistency and added a getter/setter property shadows for the active model within the ModelInstanceCollection to edit shadows in runtime instead of only on creation.
|
Already, commited a new version with the proposed changes (the getter/setter property being quite the useful one, I only thought on the basic level.). Regarding the CLA, I hope it'll be filled in soon. (I am not in a position to make that). If any problems are found I'll happily edit them. Unfortunately our timezones are a bit apart so it might take a while. |
| @@ -165,6 +167,12 @@ define([ | |||
| return this._readyPromise.promise; | |||
| } | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
Add a comma at the end of the readyPromise block and remove the empty line
| shadows : { | ||
| get : function () { | ||
| return this._model.shadows; | ||
| } |
There was a problem hiding this comment.
You should also add a set function that sets this._model.shadows.
| @@ -70,6 +70,7 @@ define([ | |||
| * @param {Boolean} [options.show=true] Determines if the collection will be shown. | |||
| * @param {Boolean} [options.allowPicking=false] When <code>true</code>, each glTF mesh and primitive is pickable with {@link Scene#pick}. | |||
| * @param {Boolean} [options.asynchronous=true] Determines if model WebGL resource creation will be spread out over several frames or block until completion once all glTF files are loaded. | |||
| * @param {ShadowMode} [options.shadows=ShadowMode.ENABLED] Determines whether the model casts or receives shadows from each light source. | |||
There was a problem hiding this comment.
Change 'model' to 'collection'.
|
Updated it again. |
|
We have a CLA from @HeercoGrond. Thanks again for the contribution! |
| return this._model.shadows; | ||
| } | ||
| set : function () { | ||
| this._model.shadows = shadows; |
There was a problem hiding this comment.
After closer review I noticed that setting the shadows dynamically is actually more involved that setting the model's shadow property. Would you mind removing this getter/setter for now? I'll be happy to merge the rest of the changes and should have a different PR open soon.
|
Removed the Getter/Setter Property for shadows. Hope for now the base of it all is done. |
|
Looks good! Thanks @HeercoGrond. |
|
Can you also add your company and yourself to |
Per Request - Added our Organization details and my name to Contributors.md
This is a possible solution for Issue #4340.