Fix scene not clearing in some Default Rendering Pipeline with multicamera cases #12905
Conversation
…ges in defaultRenderingPipeline, to ensure we call autoClear whenever we set more than 2 active cameras and have post-processes.
packages/dev/core/src/PostProcesses/RenderPipeline/Pipelines/defaultRenderingPipeline.ts
Outdated
Show resolved
Hide resolved
|
Really appreciate the use of Reflect and Proxy. I agree with @RaananW let's make that part of tools just in case |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
@sebavan sharing the implementation with the render target texture now :D |
|
Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s). |
|
Snapshot stored with reference name: Test environment: To test a playground add it to the URL, for example: https://babylonsnapshots.z22.web.core.windows.net/refs/pull/12905/merge/index.html#WGZLGJ#4600 Links to test babylon tools with this snapshot: https://playground.babylonjs.com/?snapshot=refs/pull/12905/merge To test the snapshot in the playground with a playground ID add it after the snapshot query string: https://playground.babylonjs.com/?snapshot=refs/pull/12905/merge#BCU1XR#0 |
|
@RaananW for final merge ? |
| return Reflect.set(target, property, value); | ||
| } | ||
|
|
||
| private static _ProxyPushOrUnshift<T>(operation: "push" | "unshift", observable: Observable<INotifyArrayChangeType<T>>, target: Array<T>, ...values: Array<T>): number { | ||
| observable.notifyObservers({ target, values, operation }); | ||
| observable.notifyObservers({ target, previousLength: target.length }); |
There was a problem hiding this comment.
Note that for the renderList array of the RenderTargetTexture class it is a change in behaviour: the push operation was done before the additional code was executed. Here, the additional code will be execute first (through the notifyObservers call) and the push operation will be done afterwards.
I don't know if it will break something or not, but it's for sure a different behaviour. To keep the existing behaviour it would be safer to notify the observers after the operation has been done.
Also, I think it should be wise to always notify the observers at the same point (either before or after the operation on the array has been performed) for the sake of clarity. Currently:
- set, push/unshift, delete: observers notified before the operation
- pop/shift, splice: observers notified after the operation
Or maybe it is on purpose? In that case, adding some doc on MakeObservableArray explaining when the observers are notified in each case would probably be a good idea.
| } | ||
| private _startObservingRenderListEvents() { | ||
| this._renderListHasChangedObserver = this._renderListHasChangedObservable.add(({ target, previousLength }) => { | ||
| if (target && previousLength && ((target.length > 0 && previousLength === 0) || (target.length === 0 && previousLength > 0))) { |
There was a problem hiding this comment.
I think you want to check previousLength !== undefined and not simply previousLength in target && previousLength
| return Reflect.deleteProperty(target, property); | ||
| } | ||
|
|
||
| private static _ProxyPopOrShift<T>(operation: "pop" | "shift", observable: Observable<INotifyArrayChangeType<T>>, target: Array<T>) { | ||
| const value = operation === "pop" ? Array.prototype.pop.apply(target) : Array.prototype.shift.apply(target); | ||
| observable.notifyObservers({ target, operation, values: [value] }); | ||
| observable.notifyObservers({ target, previousLength: target.length }); |
There was a problem hiding this comment.
You should get the previous length before doing the operation and pass it to the observers, as previousLength: target.length will in reality pass the current length.
| return value; | ||
| } | ||
|
|
||
| private static _ProxySplice<T>(observable: Observable<INotifyArrayChangeType<T>>, target: Array<T>, start: number, deleteNumber: number, ...added: Array<T>) { | ||
| const values = Array.prototype.splice.apply(target, [start, deleteNumber, added]); | ||
| observable.notifyObservers({ target, values, operation: "splice" }); | ||
| observable.notifyObservers({ target, previousLength: target.length }); |
carolhmj
deleted the
fixSceneClearWhenPostProcessRTTCameraIsDifferentThanSceneCamera
branch
…amera cases (#12905) * Fixes to scene clear when there are multiple cameras and post-processes. * Finish observable on activeCameras property and respond to these changes in defaultRenderingPipeline, to ensure we call autoClear whenever we set more than 2 active cameras and have post-processes. * Move observable array logic to a separate scene * Code Review * Update rendertargettexture to use the new observable array. Former-commit-id: 01910c8afd4b39194841fe06b78db52741d7fa45
Examples of where this would happen: