-
Notifications
You must be signed in to change notification settings - Fork 208
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
Add extra cleanup functionality in PrimitiveTool #6603
base: master
Are you sure you want to change the base?
Conversation
This feels like whack-a-mole. Isn't the root problem that you've got IModelApp inside some React state that's repeatedly getting recreated? |
You're right that it is whack-a-mole, but we can't dismiss the presence of moles, and the need to whack when we've found them. The root problem isn't the IModelApp being recreated - that's not the problem we're seeing anymore now that we are explictly calling onShutdown() in our viewer components. The problem is the instances where a viewport is unmounted and we want to dispose of the resources that the allocated ScreenViewport uses. The unmounting event comes from dropViewport(), which can can be called outside of IModelApp being shutdown. This particular case being ViewportComponent's handleDisconnectFromViewManager() . After your comment, I went back to look at how ViewManager calls ToolAdmin's forgetViewport, and added a line where we call the current tool's onCleanup(), since the tools themselves also carry references to the viewport in use. I didn't want to set the tool's themselves to be undefined, in case the tools are to be reused in between viewports. |
Agree. You can't now require that sub-class call super when it was previously a no-op. |
@@ -1880,8 +1882,10 @@ export class ToolAdmin { | |||
public async callOnCleanup() { | |||
await this.exitViewTool(); | |||
await this.exitInputCollector(); | |||
if (undefined !== this._primitiveTool) | |||
if (undefined !== this._primitiveTool) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can just call:
await this.setPrimitiveTool(undefined);
A continuation of the memory leak we see on refreshes and navigation away from a viewer.
In ToolAdmin, we see PrimitiveTool objects still carrying references to targetView, a Viewport object, preventing Viewport objects to be garbage collected, even after the ViewManager calls onShutdown().
This PR removes that reference to the viewport object when the PrimitiveTool's cleanup function is called, and also adds this cleanup functionality to the ToolAdmin's onShutdown().
Also, refactored the onCleanup() of subclasses extending from PrimitiveTool, to call super.cleanup() after custom cleanup functionality - Some custom cleanup functionality might rely on the iModel the tool is associated with, which is accessed through the renderTarget's reference. We don't want to remove that reference until after custom cleanup functionality from the subclasses have ran.