Skip to content

Conversation

@cournoll
Copy link
Contributor

@cournoll cournoll commented Jun 9, 2025

By using the new function findDominantDirection() of IBL shadows feature, this PR try to improves the orientation of the shadow map's light source.

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 9, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 9, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 9, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 9, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 9, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 9, 2025

Popov72 and others added 2 commits June 9, 2025 16:56
… generator task (BabylonJS#16734)

Improved fix for BabylonJS#16710:
* The depth texture to use for min/max reduction is now an input of the
CSM task. This means that this depth texture can be shared by multiple
tasks. In the previous PR, a new depth texture was always generated
internally when `autoCalcDepthBounds` was **true**
* The textures used for min/max reduction are created as part of the
frame graph, which means that these textures can be optimized and reused
by the graph

I have also added support for “normalized view depth” as an output of
the geometry renderer task, as this is the most optimal type for min/max
depth reduction when used in CSM.
The previous light direction calculation didn't account for when the
Babylon light direction matched glTF's. This would result NaN values in
RotationAxisToRef, as the rotation axis would be (0,0,0), and its length
(0) used as a divisor.

I noticed we already have a function to do what we need for the light
direction calculation-- `FromUnitVectorsToRef`-- so I've replaced the
whole thing with that. Much simpler.

(Bonus: I also noticed that the lights in our glTF light export test
don't use glTF falloff, so I've earmarked that change in this PR.)
ryantrem and others added 3 commits June 9, 2025 18:10
This PR introduces a test app for local development of Inspector v2. It
uses webpack, but a newer version than our other tools which is needed
for compatibility with the webpack React fast refresh plugin (which is a
new local dev only dependency). The simple test app just loads a model,
and also enables drag & drop to easily test other models. Inspector is
always open, and fast refresh is supported so as you make changes to
React components, you immediately see them in the test app, without
reloading the page or re-creating the scene. You can run and debug the
test app from VSCode by just running the new "Inspector v2 development
(Chrome)" launch command. This enables a very tight dev loop. Hopefully
in the future we can see what it would take to bring fast refresh to
other tools, like Sandbox and Playground, but for now this is a low cost
solution to help speed up Inspector v2 development.
- Remove redundant calls of normalize.
- Fix type of normal.light.
- Added `_throwIfDisposedOrAborted` to ensure timely termination if the parent operation is aborted.
@bjsplat
Copy link
Collaborator

bjsplat commented Jun 10, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 10, 2025

Comment on lines 1805 to 1808
this._iblDirectionCalculationAbortController?.abort(new AbortError("New IBL dominant direction calculation initiated before previous calculation finished."));
const iblDirectionCalculationAbortController = (this._iblDirectionCalculationAbortController = new AbortController());

return await this._iblDirectionCalculationLock.lockAsync(async () => {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh dang, sorry, I didn't realize in my first review pass that this function is only called through the _updateShadows code path, which already has a lock and an abort controller. If that is really the only place it will be called, and this function is really just part of the implementation of _updateShadowMap (just factored out into its own function for readability), then it doesn't need its own abort controller and lock. It only needs those if it is potentially called from multiple call sites that are not under the same lock. Sorry for the confusion. :/

return this._cachedIblDirection;
}

this._cachedIblDirection = null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iblCdfGenerator already caches the dominant direction so calling findDominantDirection multiple times should just return right away without recalculating the direction.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another thing to keep in mind is that the direction returned here is unrotated. i.e. if the IBL has a rotationY value other than 0, this vector should be manually rotated to match the IBL rotation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and the magnitude of the direction returned is an indication of how dominant the direction is. So you could, in theory, use the magnitude to adjust the darkness and sharpness of the shadow.
A magnitude near 1.0 would indicate that the light is very strongly coming from one direction. A magnitude closer to 0.0 indicates that the light is coming from lots of directions.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

iblCdfGenerator already caches the dominant direction so calling findDominantDirection multiple times should just return right away without recalculating the direction.

Yes i knew it, my main motivation for adding this._cachedDominantDirection into the viewer was to allow the _rotateShadowLightWithEnvironment function to remain synchronous (because I feel like it gets called a lot of times when it's plugged into a slider).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, and the magnitude of the direction returned is an indication of how dominant the direction is. So you could, in theory, use the magnitude to adjust the darkness and sharpness of the shadow. A magnitude near 1.0 would indicate that the light is very strongly coming from one direction. A magnitude closer to 0.0 indicates that the light is coming from lots of directions.

Oh nice thx for the info !

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 11, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 11, 2025

The iblLightStrength value is derived from the magnitude of the IBL's dominant light direction to enhance shadow realism by tweaking ShadowGenerator.darkness and ShadowGenerator.blurKernel.
@bjsplat
Copy link
Collaborator

bjsplat commented Jun 11, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 11, 2025

amirt-ms and others added 19 commits June 11, 2025 23:30
…ylonJS#16737)

I am working on a project that uses the ArcRotateCamera in Orthographic
mode. I want to use the `ArcRotateCamera.zoomOn` function to zoom to a
set of meshes. Unfortunately, the zoomOn function updates only the
radius, not the orthographic extents.

This PR updates the orthographic extents along with the radius when
zooming onto mesh(es) in orthographic mode.

Here is a demo playground: https://playground.babylonjs.com/#545IZ4

**Before:**


https://github.com/user-attachments/assets/77b8516e-4ec8-40e0-a4d5-4f9ae0d4104d

**After:**


https://github.com/user-attachments/assets/876c3b68-3638-48a2-9b71-c909a4ae55ef
There's one color attribute declaration that is missing and will cause
an error.

---------

Co-authored-by: Kevin Blackburn-Matzen <matzen@adobe.com>
- [x] On end trigger
- [x] On start trigger
- [x] support worldMatrix from emitter
- [x] Preview popup
- [x] Flow map
- [x] Expose emitter
- [x] Sprite sheet
- [x] Delay start
- [x] Particle trigger
- [x] Condition
- [x] Simple condition 
- [x] Auto expendable blocks
- [x] Basic update
- [x] Snippet & loading API
- [x] Teleport
- [x] Elbow
- [x] Custom Emitter Type
- [x] Trigonometrics
- [x] Converter



![image](https://github.com/user-attachments/assets/7c1c1de0-780e-4074-ba48-26282b4a2eff)

---------

Co-authored-by: David Catuhe <david@catuhe.com>
- Remove redundant calls of normalize.
- Fix type of normal.light.
- Added `_throwIfDisposedOrAborted` to ensure timely termination if the parent operation is aborted.
The iblLightStrength value is derived from the magnitude of the IBL's dominant light direction to enhance shadow realism by tweaking ShadowGenerator.darkness and ShadowGenerator.blurKernel.
@bjsplat
Copy link
Collaborator

bjsplat commented Jun 12, 2025

You have made possible changes to the playground.
You can test the snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16735/merge/

The snapshot playground with the CDN snapshot (only when available):

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/PLAYGROUND/refs/pull/16735/merge/?snapshot=refs/pull/16735/merge

Note that neither Babylon scenes nor textures are uploaded to the snapshot directory, so some playgrounds won't work correctly.

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 12, 2025

You have changed file(s) that made possible changes to the sandbox.
You can test the sandbox snapshot here:

https://snapshots-cvgtc2eugrd3cgfd.z01.azurefd.net/SANDBOX/refs/pull/16735/merge/

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 12, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Jun 12, 2025

@cournoll cournoll marked this pull request as draft June 12, 2025 12:06
@deltakosh
Copy link
Contributor

Should we close that one @ryantrem ?

@ryantrem ryantrem closed this Jun 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants