Post effect sketches without root properly reload #473
Conversation
There was a problem hiding this comment.
Thanks for this, it's a great catch when it comes to the postprocessing stuff. Generally I like this approach but i've left a few comments below. The most important being a big question of whether we want to be able to mix 3D object sketches with post processing sketches? I can imagine a sketch having both combined, but maybe that's a bad idea. Curious to hear your thoughts.
|
|
||
| if (!oldSketch) { | ||
| const pass = engineScene.getPassesByName(instanceId) | ||
| if (pass?.length) { | ||
| pass.forEach((p) => engineScene.removePass(p)) |
There was a problem hiding this comment.
I think this would also work 😉
| pass.forEach((p) => engineScene.removePass(p)) | |
| pass.forEach(engineScene.removePass) |
|
|
||
| if (!oldSketch) { | ||
| const pass = engineScene.getPassesByName(instanceId) |
There was a problem hiding this comment.
Do we need this method? Can we not just do this.sketchInstances[instanceId].getPasses? Maybe you have a good reason for doing it this way though, curious to know.
| delete this.sketchInstances[instanceId] | ||
| return | ||
| } | ||
| throw new Error(`couldn't find sketch to remove: ${instanceId}`) | ||
| } | ||
|
|
||
| scene.remove(oldSketch) | ||
| threeScene.remove(oldSketch) |
There was a problem hiding this comment.
Wondering if we may need to still call threeScene.remove sometimes even when there are passes on the sketch?
Is it not quite likely that some sketches may have both postprocessing and 3D objects? Rather than immediately returning at the end of the passes stuff, I'm wondering if it makes sense to shuffle the logic a bit so the code keeps keeps executing to the end of the method. You could optionally call threeScene.remove(oldSketch). Also wouldn't need to repeat the delete this.sketchInstances[instanceId] as it will just naturally get there anyway.
If a sketch does not have an object named root, it fires an error when a code change is detected, requiring the user to manually remove and read the sketch.
This change checks to see if the refreshing sketch is one of the current post effects, and breaks early if so
accidentally added this off #471 so review that first 😬