Complete scene realization and rendering support for interactive demos - #296
Conversation
|
The latest updates on your projects. Learn more about Argos notifications ↗︎
|
bdero
left a comment
There was a problem hiding this comment.
dart analyze is clean and all 1121 flutter_scene tests pass locally. The mesh-variant fix, the nested override grammar, and the layer-mask query coverage are all solid.
Three things look like blockers.
- The rapier ABI change ships without the release that makes it safe.
wasm_release.dartstill pins the 0.4.0 module, and the web path has no ABI gate, so every query passeslayerMaskwhere the old export expects the out pointer.native_binaries.jsonstill declaresabi_version: 1against the hook's new2, so native consumers fall through to a source build that is currently broken on Android. The package is still 0.4.0 with no CHANGELOG entry. This wants to be its own PR with its own release cycle. - glTF punctual light intensity is imported raw. The extension specifies candela and lux, while
PointLight.intensityis an artistic radiance-at-unit-distance scalar, so authored lights land orders of magnitude too bright. It also turns on for every existingbuildScenesuser with no opt-out, and the runtime importer does not do it, so the two import paths now disagree about what a.glbcontains. - Most of the new public API is missing from the CHANGELOG.
dart_apitoolwill list all of it, worth running now rather than at release.
On the API design, Material.depthBias is the one I would most want to change before it is public. It is a raw NDC offset, so its meaning varies with the backend's clip depth convention, the depth attachment precision, and the scene's world scale. It also goes into .fscene documents as an authored property, so the unit is expensive to change later. A view-space offset converted per draw, or a normalized count of depth-buffer units, would stay meaningful across all three.
The rest is inline. Most of it is small.
bdero
left a comment
There was a problem hiding this comment.
Re-reviewed at 85256be. dart analyze is clean across all four packages and 1124 tests pass.
The three blockers are properly resolved. Backing the rapier layer-mask change out entirely was the right call, and the revert is complete, including restoring the TODO(query-layer-mask) so the docs no longer overstate what the parameter does. The photometric conversion through one shared helper used by both import paths is exactly right. The CHANGELOG audit looks thorough, and flagging the Geometry.bind signature as breaking is the correct read, since Dart requires overrides to accept the supertype's named parameters.
Moving depthBias to a world-space offset is a real improvement. It is now projection, depth-format, and clip-convention independent, lighting still uses the unbiased position, and the shadow pass binds zero. Making bias a bind parameter instead of geometry state closed the stale-state hazard properly.
One new issue, in _visitMutable. It is correct, but it is quadratic and allocates per node per frame, in the traversal that runs for every node every frame and every fixed substep. Measured on this machine, one 1024-element list costs 784us per pass and 4096 costs 12.1ms, against 6.9us and 23.8us for the same guarantees with a fast-path identity check. That is a scene-killing regression for exactly the wide-sibling case the engine is meant to scale to, so I would not land it as written. Details inline.
The rest is follow-ups and two things worth eyeballing on the smoke lane.
bdero
left a comment
There was a problem hiding this comment.
Re-reviewed at 33b6b72. This looks good to merge on substance.
dart analyze is clean across flutter_scene, scene, flutter_scene_rapier, and flutter_scene_editor_core, and 1124 tests pass.
The traversal fix works. Measured again on the same bench, an unmutated 4096-child list went from 12.1ms to 25us, and 16384 children now cost 98us. The fast path allocates nothing, and seeding the tracking set from the current prefix is sound for each of the documented mutation shapes, including the insert-before case where the newly inserted node is correctly held back to the next frame. Reordering is the one case that still skips siblings, and documenting it as unsupported is the right call given the cost of handling it.
Fading subpixel taps with smoothstep while leaving weight_sum complete is a better fix than dropping them, since the denominator stays stable and distant support ramps to zero instead of stepping. The shared depth_bias.glsl and interleaved_gradient_noise.glsl includes match how pbr.glsl and material_lighting.glsl already work, so they need no manifest entry.
The rapier change is now just normalizing the cached prebuilt's filename through targetOS.dylibFileName, with a test. No ABI change, no manifest change, no release coupling. That is the right place to have landed.
One performance follow-up inline, worth an issue rather than another round. Three mechanical gates before merging are in the summary below.
Merge gates, none of them about the code:
- The branch is
CONFLICTINGagainst master. The conflict is onlyCHANGELOG.md, where master'sa28bd88added a bullet to the same 0.21.0 section. A rebase resolves it. - CI's last green run is 85256be, not 33b6b72. Worth letting Flutter CI and smoke render finish on the final commit.
flutter_scene_netnow requiresdashwire_replication ^0.2.0while its published 0.1.0 pins^0.1.0, which overlaps what stage B in #288 is scoped to do. Worth deciding which PR carries the bump so the two do not fight, and rememberingflutter_scene_netneeds a release either way.
33b6b72 to
7a89a1a
Compare
|
Rebased onto current master. This PR carries the dependency constraint bump required by the demo, while the networking PR can inherit it and the package can release with its next version. |
Adds linked FScene realization, authored material loading, filtered physics queries, safe graph mutation, and imported mesh variant handling. Improves ambient occlusion, tone mapping, post-processing stage application, networking, and native asset compatibility for large interactive scenes.