-
Notifications
You must be signed in to change notification settings - Fork 152
Canvas and Context #1491
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
Canvas and Context #1491
Conversation
Fix build/installation regression introduced with BabylonJS#1472
Updated Babylon.js package to 8.0
## Cleared resources Textures are zeroed by browsers by default:   It's not the case with Native  With Canvas:  ## Light projection texture Nothing rendered  Fixed by checking rendertargets with `executeWhenReady`
bghgary
left a comment
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.
Looks good except for the circular reference
Apps/UnitTests/Scripts/tests.js
Outdated
| var texSize = 512; | ||
| var dynamicTexture = new BABYLON.DynamicTexture("dynamic texture", texSize, scene); | ||
| var context = dynamicTexture.getContext(); | ||
| var otherContext = dynamicTexture.getContext(); |
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.
why var and not const?
| return Context::CreateInstance(info.Env(), this); | ||
| if (m_contextObject.IsEmpty()) | ||
| { | ||
| m_contextObject = Napi::Persistent(Context::CreateInstance(info.Env(), info.This()).As<Napi::Object>()); |
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.
Canvas holds context and context holds canvas. This is probably going to prevent the GC from collecting these objects once created. Can you check?
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.
With canvasObject persistent, context is not disposed nor Canvas when disposing the dynamic texture. When using Napi::Weak Canvas and Context are properly disposed when disposing dyn. texture.
Polyfills/Canvas/Source/Context.cpp
Outdated
| Context::Context(const Napi::CallbackInfo& info) | ||
| : Napi::ObjectWrap<Context>{info} | ||
| , m_canvas{info[0].As<Napi::External<NativeCanvas>>().Data()} | ||
| , m_canvasObject{Napi::Weak(info[0].As<Napi::Object>())} |
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.
Does there need to be a js reference from context to canvas? If the js side only references the context, we don't want the canvas to be gc'd, right? I assume this is how it works in the browser?
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.
The reference from context to canvas is needed by the getCanvas method.
If the js side only references the context, we don't want the canvas to be gc'd, right? I assume this is how it works in the browser?
That was my guess. But in that case, when is the Canvas and Context are collected if both keep a strong reference on each other? Is there a way to detect that?
… into ContextGetCanvas # Conflicts: # Apps/package-lock.json # Apps/package.json # Polyfills/Canvas/Source/Canvas.cpp # Polyfills/Canvas/Source/Context.cpp # Polyfills/Canvas/Source/Context.h
Replaces #1488
CreateInstancewithInitializeGetCanvasgetContext