Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix disposing of effects for submeshes #15910

Merged
merged 11 commits into from
Nov 29, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions .build/config.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{
"versionDefinition": "minor",
"preid": "rc",
"nonce": 298
}
"versionDefinition": "minor",
"preid": "rc",
"nonce": 299
}
2 changes: 1 addition & 1 deletion packages/dev/core/src/Engines/thinEngine.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4037,7 +4037,7 @@ export class ThinEngine extends AbstractEngine {
const keys = Object.keys(this._compiledEffects);
for (const name of keys) {
const effect = this._compiledEffects[name];
effect.dispose();
effect.dispose(true);
}

this._compiledEffects = {};
Expand Down
19 changes: 17 additions & 2 deletions packages/dev/core/src/Materials/drawWrapper.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import type { Nullable } from "../types";
import type { AbstractEngine } from "../Engines/abstractEngine";
import type { Effect } from "./effect";
import type { MaterialDefines } from "./materialDefines";
import { TimingTools } from "core/Misc/timingTools";

/** @internal */
export class DrawWrapper {
Expand Down Expand Up @@ -54,9 +55,23 @@ export class DrawWrapper {
}
}

public dispose(): void {
/**
* Dispose the effect wrapper and its resources
* @param immediate if the effect should be disposed immediately or on the next frame.
* If dispose() is not called during a scene or engine dispose, we want to delay the dispose of the underlying effect. Mostly to give a chance to user code to reuse the effect in some way.
*/
public dispose(immediate = false): void {
if (this.effect) {
this.effect.dispose();
const effect = this.effect;
if (immediate) {
effect.dispose();
} else {
TimingTools.SetImmediate(() => {
effect.getEngine().onEndFrameObservable.addOnce(() => {
effect.dispose();
});
});
}
this.effect = null;
}
this.drawContext?.dispose();
Expand Down
16 changes: 14 additions & 2 deletions packages/dev/core/src/Materials/effect.ts
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,13 @@ export class Effect implements IDisposable {

private _isDisposed = false;

/**
* Gets a boolean indicating that the effect was already disposed
*/
public get isDisposed(): boolean {
return this._isDisposed;
}

/** @internal */
public _refCount = 1;

Expand Down Expand Up @@ -1466,9 +1473,14 @@ export class Effect implements IDisposable {

/**
* Release all associated resources.
* @param force specifies if the effect must be released no matter what
**/
public dispose() {
this._refCount--;
public dispose(force = false) {
if (force) {
this._refCount = 0;
} else {
this._refCount--;
}

if (this._refCount > 0 || this._isDisposed) {
// Others are still using the effect or the effect was already disposed
Expand Down
16 changes: 9 additions & 7 deletions packages/dev/core/src/Meshes/abstractMesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -670,7 +670,7 @@ export abstract class AbstractMesh extends TransformNode implements IDisposable,
return;
}

this.resetDrawCache();
this.resetDrawCache(undefined, value == null);
this._unBindEffect();
}

Expand Down Expand Up @@ -1257,14 +1257,15 @@ export abstract class AbstractMesh extends TransformNode implements IDisposable,
/**
* Resets the draw wrappers cache for all submeshes of this abstract mesh
* @param passId If provided, releases only the draw wrapper corresponding to this render pass id
* @param immediate If true, the effect will be released immediately, otherwise it will be released at the next frame
*/
public resetDrawCache(passId?: number): void {
public resetDrawCache(passId?: number, immediate = false): void {
if (!this.subMeshes) {
return;
}

for (const subMesh of this.subMeshes) {
subMesh.resetDrawCache(passId);
subMesh.resetDrawCache(passId, immediate);
}
}

Expand Down Expand Up @@ -2180,13 +2181,14 @@ export abstract class AbstractMesh extends TransformNode implements IDisposable,
}

/**
* Disposes all the submeshes of the current meshnp
* Disposes all the submeshes of the current mesh
* @param immediate should dispose the effects immediately or not
* @returns the current mesh
*/
public releaseSubMeshes(): AbstractMesh {
public releaseSubMeshes(immediate = false): AbstractMesh {
if (this.subMeshes) {
while (this.subMeshes.length) {
this.subMeshes[0].dispose();
this.subMeshes[0].dispose(immediate);
}
} else {
this.subMeshes = [] as SubMesh[];
Expand Down Expand Up @@ -2283,7 +2285,7 @@ export abstract class AbstractMesh extends TransformNode implements IDisposable,

// SubMeshes
if (this.getClassName() !== "InstancedMesh" || this.getClassName() !== "InstancedLinesMesh") {
this.releaseSubMeshes();
this.releaseSubMeshes(true);
}

// Query
Expand Down
16 changes: 9 additions & 7 deletions packages/dev/core/src/Meshes/subMesh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -61,9 +61,9 @@ export class SubMesh implements ICullable {
/**
* @internal
*/
public _removeDrawWrapper(passId: number, disposeWrapper = true) {
public _removeDrawWrapper(passId: number, disposeWrapper = true, immediate = false): void {
if (disposeWrapper) {
this._drawWrappers[passId]?.dispose();
this._drawWrappers[passId]?.dispose(immediate);
}
this._drawWrappers[passId] = undefined as any;
}
Expand Down Expand Up @@ -114,15 +114,16 @@ export class SubMesh implements ICullable {
/**
* Resets the draw wrappers cache
* @param passId If provided, releases only the draw wrapper corresponding to this render pass id
* @param immediate If true, the draw wrapper will dispose the effect immediately (false by default)
*/
public resetDrawCache(passId?: number): void {
public resetDrawCache(passId?: number, immediate = false): void {
if (this._drawWrappers) {
if (passId !== undefined) {
this._removeDrawWrapper(passId);
this._removeDrawWrapper(passId, true, immediate);
return;
} else {
for (const drawWrapper of this._drawWrappers) {
drawWrapper?.dispose();
drawWrapper?.dispose(immediate);
}
}
}
Expand Down Expand Up @@ -697,8 +698,9 @@ export class SubMesh implements ICullable {

/**
* Release associated resources
* @param immediate If true, the effect will be disposed immediately (false by default)
*/
public dispose(): void {
public dispose(immediate = false): void {
if (this._linesIndexBuffer) {
this._mesh.getScene().getEngine()._releaseBuffer(this._linesIndexBuffer);
this._linesIndexBuffer = null;
Expand All @@ -708,7 +710,7 @@ export class SubMesh implements ICullable {
const index = this._mesh.subMeshes.indexOf(this);
this._mesh.subMeshes.splice(index, 1);

this.resetDrawCache();
this.resetDrawCache(undefined, immediate);
}

/**
Expand Down