Skip to content

fix(editor): swap models in editor.save() without blank-frame flicker#208

Open
rihokirss wants to merge 1 commit intoThatOpen:mainfrom
rihokirss:fix/editor-save-flicker
Open

fix(editor): swap models in editor.save() without blank-frame flicker#208
rihokirss wants to merge 1 commit intoThatOpen:mainfrom
rihokirss:fix/editor-save-flicker

Conversation

@rihokirss
Copy link
Copy Markdown

@rihokirss rihokirss commented May 9, 2026

Closes #207.

Summary

editor.save(modelId) previously disposed the outgoing model — removing its THREE.Object3D from the scene and tearing down geometry, materials, alignments and grids — before loading the replacement. This left a one-or-more-frame window with nothing rendered for that modelId, producing a visible flicker.

This PR reorders save() so the replacement is fully loaded and added to the scene before the old model's visuals are torn down. See #207 for the full diagnosis.

Changes

  • FragmentsModel.dispose({ keepInScene }) — phase 1 of a deferred dispose. Frees only the worker slot, the meshes.list entry, the threads routing entry, and the model's slot in the shared MaterialManager so the replacement can register under the same modelId. The THREE.Object3D, tile meshes, materials in list, alignments and grids are left untouched and continue rendering.

  • MaterialManager.releaseModelSlot(modelId) — clears _definitions[modelId] and _modelMaterialMapping[modelId] without disposing the underlying THREE materials in list. Required because addDefinitions() appends to any existing array under that modelId; leaving the outgoing slot in place would corrupt the replacement model's material indices when its CREATE_MATERIAL requests arrive.

  • FragmentsModel.finalizeDispose() — phase 2, called after the replacement is in the scene. Does removeFromParent(), disposes per-tile geometries, and disposes alignments + grids. It deliberately does not dispose materials: they are CRC-keyed (with modelId in the CRC) and getUniqueMaterial returns existing entries from list, so the replacement model frequently reuses the same instances. Picker materials also flow through MaterialManager.list, so disposing them here would silently break picking/highlighting/raycasting on the replacement model. The tile map is cleared with events disabled to bypass the constructor's onBeforeDelete listener (which would otherwise dispose tile materials).

  • EditHelper.save() — uses the two phases:

    await model.dispose({ keepInScene: true });  // phase 1
    const newModel = await this._fragments.load(...);
    await newModel._setRequests({ undoneRequests: requests.undoneRequests });
    if (parent) parent.add(newModel.object);
    model.finalizeDispose();                      // phase 2

Behaviour change

  • Public API of dispose() is backward compatible — keepInScene is a new optional flag that defaults to false.
  • finalizeDispose() is new public surface; it is only meaningful after dispose({ keepInScene: true }).
  • MaterialManager.releaseModelSlot() is new public surface.

Verification

Tested end-to-end against fragments built from this branch:

  • Paste-on-element flow that calls editor.save() on every commit no longer flickers.
  • Picking, highlighting and raycasting continue to work on the replacement model.
  • Subsequent edits and saves keep working across many cycles.

Build is bit-identical to the published @thatopen/fragments@3.4.5 for the unchanged code paths (verified via md5 on built dist/ files before applying this patch).

editor.save() previously disposed the outgoing model (removing its THREE
object from the scene and tearing down geometry, materials, alignments
and grids) before loading the replacement. This left a window of one or
more frames where nothing was rendered for the model, producing a
visible flicker — most noticeable for users who edit elements often
(e.g. paste/move flows that commit a delta back into the main model).

This commit reorders save() so the new model is fully loaded and added
to the scene before the old model's visuals are torn down.

Implementation:

  * FragmentsModel.dispose now accepts { keepInScene } which, in phase 1,
    frees only the worker slot, the meshes.list entry, the threads
    routing entry, and the model's slot in the shared MaterialManager —
    so the replacement model can register under the same modelId. The
    THREE object, tile meshes, materials in MaterialManager.list,
    alignments and grids are left untouched and continue rendering.

  * MaterialManager.releaseModelSlot(modelId) clears the per-model
    definitions and mapping without disposing the underlying THREE
    materials. This is necessary because addDefinitions() *appends* to
    any existing array under that modelId, so leaving the outgoing
    model's slot in place would corrupt the replacement model's material
    indices and break tile lookups.

  * FragmentsModel.finalizeDispose() does the deferred visual cleanup
    after the new model is in scene: removeFromParent, geometry disposal
    for each tile, then alignments/grids dispose. It does NOT dispose
    materials. Materials are CRC-keyed (with modelId in the CRC) and
    getUniqueMaterial returns existing entries from list, so the
    replacement model frequently reuses the same THREE material
    instances. Disposing them here would silently break the replacement
    model's rendering and picker — picker materials also flow through
    MaterialManager.list. Tile material disposal in the constructor's
    onBeforeDelete listener is bypassed by clearing the tile map with
    events disabled.

Reported in https://people.thatopen.com/c/ask-the-community/edited-element-stays-in-delta-model-after-transform-edit-causing-picking-highlight-issues-unless-editor-save-is-called
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.

editor.save() causes a visible blank-frame flicker because the model is disposed before the replacement is loaded

1 participant