Plugin Refactor by syntheticmagus · Pull Request #157 · BabylonJS/BabylonNative

syntheticmagus · 2020-02-20T02:59:40Z

This is the refactor to convert Babylon Native to relying on plugins as its preeminent variety of component. This change is intended to improve scalability by "flattening" the structure of the repository, moving certain responsibilities outside the library and allowing a looser coupling of components. This change is by no means finished; but with the build system working and the apps of the four primary Windows builds confirmed to run, it is now at least ready to be looked at (and, if desired, downloaded and tried).

…here we want it long-term.

…ructure!

…Impl's responsibilities.

…Runtime's hideable behaviors ended up getting consumed by the TaskChain.

ryantrem · 2020-02-20T17:07:25Z

+    auto* windowPtr = reinterpret_cast<ABI::Windows::UI::Core::ICoreWindow*>(CoreWindow::GetForCurrentThread());
+    m_runtime->Dispatch([windowPtr, width, height](Napi::Env env)
+    {
+        Babylon::NativeWindow::Initialize(env, windowPtr, width, height);


Thinking about how this will work later with multi-view... maybe we would initialize the NativeWindow with just the env, and then call a function on it like RegisterView that takes the windowPtr, width, and height? Is it worth trying to get the right API now even if we only support one window/view?

This is actually an analogue to the JavaScript concept of the Window, so this particular aspect of it likely won't change for multiview scenarios as I think that's a separate issue. This is one of the plugins we should probably consider renaming, though, to help make it more clear what exactly it is.

ryantrem · 2020-02-20T17:23:31Z

+
+namespace Babylon
+{
+    class TaskChain


Consider naming this WorkQueue (or JSWorkQueue) (I feel like that is a more standard name for this concept).

ryantrem · 2020-02-20T17:31:27Z

+
+namespace Babylon
+{
+    TaskChain::TaskChain(std::function<void()> threadProcedure)


It seems weird that this has to be passed in, and the assumption is that this function eventually calls RunTaskChain. Why can't TaskChain start it's own work queue thread, and the TaskChain consumer just call Append to get work injected into the thread?

It might be possible to do it that way; I'll have to think about it more. This is a fairly direct port of the existing mechanism, and the main reasoning behind that mechanism is because of stack variables. In particular, V8 wants its initialization and state to be stored on the stack, and it goes to great lengths to make it difficult to do anything else. Thus, to use V8 correctly, we need to actually call TaskChain::RunTaskChain from inside the scope that contains the V8 state, rather than calling a task at the start to set up V8 and presumably calling another later to tear it down.

ryantrem · 2020-02-20T17:36:55Z

+    });
+
+    // Initialize NativeEngine plugin.
+    Babylon::InitializeNativeEngine(*m_runtime, windowPtr, width, height);


I'm a bit confused why the windowPtr, width, and height need to be passed to both NativeWindow and NativeEngine.

This, in part, is just me porting over what the existing API was rather than making too many invasive API changes at this point in the refactor. However, the reason the API was that way is because of macOS and iOS. Those three values are required (in the current implementation) for the initialization of bgfx, and my understanding is that the Apple operating systems require bgfx to be initialized on the main thread, where it doesn't have access to the NativeWindow because the Napi::Env can't be accessed on that thread and may not even exist yet anyway.

To be clear, this is something I'd definitely like to think about revising, but I'm trying to keep cleanup work on the plugins themselves to a minimum in this change. I'm currently hoping to limit this PR as much as possible to restructuring, after which we can tackle the individual plugins in relative isolation.

bghgary

Looks pretty good overall. I like the separation of the plugins.

bghgary · 2020-02-20T17:13:33Z

 WCHAR szWindowClass[MAX_LOADSTRING];            // the main window class name
-std::unique_ptr<Babylon::RuntimeWin32> runtime{};
+std::unique_ptr<Babylon::AppRuntime> runtime{};
+std::string rootUrl{};


why is this global now?

Workaround for dangerous design decision that makes XMLHttpRequest::Initialize take a const char* that must outlive the JsRuntime. Touched on this in the comment I made in .../Win32/App.cpp. When that issue is fixed, this variable should also stop being global.

bghgary · 2020-02-20T17:17:47Z

+        AppRuntime(const char* rootUrl = nullptr);
+        ~AppRuntime();
+
+        const std::string& GetRootUrl() const;


We should talk about the convention for getters in our code.

Suggested change

const std::string& GetRootUrl() const;

const std::string& RootUrl() const;

Since it's const anyway, maybe we should just make the member public.

bghgary · 2020-02-20T17:21:23Z

+#if WINAPI_FAMILY == WINAPI_FAMILY_DESKTOP_APP
+        HRESULT hr = CoInitializeEx(nullptr, COINIT_APARTMENTTHREADED);
+        assert(SUCCEEDED(hr));
+        auto coInitScopeGuard = gsl::finally([] { CoUninitialize(); });
+#endif
+


This seems like it should be outside of this class and in AppRuntimeWin32 or something.

bghgary · 2020-02-20T17:26:34Z

+        std::mutex m_blockingTickMutex{};
+        std::mutex m_suspendMutex{};
+        std::condition_variable m_suspendConditionVariable{};
+        bool m_suspended{ false };


nit: might consider auto formatting all the files

bghgary · 2020-02-20T17:33:12Z

+        static constexpr auto JS_WINDOW_NAME = "window";
+    }
+
+    JsRuntime::JsRuntime(DispatchFunctionT&& dispatchFunction)


This shouldn't be a &&. There is no reason to force people to move the dispatch function into the constructor.

Suggested change

JsRuntime::JsRuntime(DispatchFunctionT&& dispatchFunction)

JsRuntime::JsRuntime(DispatchFunctionT dispatchFunction)

Use std::move like you are doing in the other constructor to move. The calling code can choose whether to move or not.

The reasoning here was that this is protected, and that in this case we actually do want to force the function to be moved in. I don't think there's ever a case where we want this function to be passed by copy because there's no reason for anybody else to have this lambda. In such a circumstance, is it better to still leave the choice up to the calling code?

bghgary · 2020-02-20T17:36:02Z

@@ -0,0 +1,131 @@
+set(CMAKE_CXX_STANDARD 17)


I'm not sure I understand the point of this file. Are we expecting that anyone who wants to use any dependencies add this as a subdirectory in CMakeLists.txt?

Spoke offline. This file allows the Dependencies folder to participate in the CMake pattern shared by the other major folders (Core, Plugins, and Apps). Most of the code in here is currently just ported directly from the old build system, but hopefully we'll improve it as we go to make it as clean and airtight as possible over time.

bghgary · 2020-02-20T17:37:25Z

@@ -0,0 +1,13 @@
+set(SOURCES
+    "Include/Babylon/NetworkUtils.h"
+    "Include/Babylon/ticketed_collection.h"


Maybe TicketedCollection.h to be consistent in naming schemes?

bghgary · 2020-02-20T17:48:56Z

+
+add_library(BabylonNativeUtils ${SOURCES})
+
+target_include_directories(BabylonNativeUtils PRIVATE "Include/Babylon")


Ugh, this naming scheme is killing me. The directory name is snake case but the library is upper camel case?

bghgary · 2020-02-20T17:49:34Z

+target_include_directories(BabylonNativeUtils INTERFACE "Include")
+
+target_link_libraries(BabylonNativeUtils
+    PUBLIC arcana


Is it expected that arcana is already added?

Yes. The pattern being established here is that plugins (and modules following the plugin pattern) do not supply their own dependencies, but instead assume they exist as a part of a larger build system that bears the responsibility of making sure all dependencies are loaded in the right order. This allows the inclusions to scale horizontally and, when applicable, share dependencies amongst themselves.

syntheticmagus · 2020-02-20T20:49:02Z

+        // Initialize XMLHttpRequest plugin.
+        runtime->Dispatch([rootUrl](Napi::Env env)
+        {
+            Babylon::XMLHttpRequest::Initialize(env, rootUrl.data());


This is dangerous and probably doesn't work at all, must have dodged tests: the char* being passed to Initialize points to a copy of rootUrl local to this lambda, which will get cleaned up long before any XMLHttpRequest objects are created. XMLHttpRequest::Initialize should make a copy of the text and put it somewhere safe.

Co-Authored-By: Gary Hsu <bghgary@users.noreply.github.com>

…NativeEngine.

syntheticmagus

Thanks for the feedback, Cedric! For some reason, I can't seem to reply to your comment directly, so here are some of the points you asked about. Short version: most of my responses resolve down to, "That's definitely something I want to fix, but in a separate PR." 😀

"Helper," to me, implies there's an object, which seems misleading in this case. I have no particular attachment to "Utils," but it is relatively standard and unsurprising: people will know the purpose that folder is serving as soon as they read that name. Is there another name you'd prefer?
ScriptLoader cannot live in Dependencies because it depends on JsRuntime. I don't think we've formally defined it, but I tend to think of a plugin not as something that necessarily extends the functionality of the JS engine, but rather as any component that (1) is designed to be reused and (2) depends on JsRuntime.
Yes, this naming is something of an antipattern. This is one of the things I want to clean up as soon as possible. However, during this particular check-in, I'm trying to avoid changing the implementations of the plugins themselves in order to limit the scope of the check-in to structure as much as possible. Because of the amount of exposure in the "real" NativeEngine.h (and also XMLHttpRequest.h, which I also moved to this pattern earlier today) was problematic, I created this two-header alternative as a temporary workaround. It should never be possible to confuse the headers -- one must always be referred to as Babylon/NativeEngine.h while the other can only be accessed as NativeEngine.h, but it's still something I want to remove as one of the first things I do to clean up the plugins. With that in mind, do you think it's fine for this temporary workaround to get checked in to avoid getting even more scope creep in this PR?
Probably. I hadn't really thought about it that way, and I think we'd need to change the way we're thinking about N-API to make that work, but it might be something explore. I think I'd rather we explore that in a separate PR, though, as it seems like it might go deep. Also, regarding using other JS engines, the current architecture still allows us to swap amongst them, it's just not done using the plugin mechanism.
The ScriptLoader at this point is pretty directly just a port of some existing capabilities that were originally on the Runtime. We can modify it to be platform-adaptive, or make platform-specific versions of it, or something like that if we want to. For now, though, all it did was consume the Runtime's pre-existing dependency on the network utils.
Yep, the plugin initialization code is a mess. That's another artifact of my reluctance to change the plugins themselves any more than I have to as a part of this check-in. My hope is that, as soon as this change is in, I'll be able to embark on a sequence of PRs tackling the different plugins individually, one by one; getting the initialization code to all conform to a consistent pattern is one of the things I'd fix in those PRs. It's definitely a lot uglier and more complicated than it needs to be right now; I'm just not sure this particular PR is the right place to fix that.

syntheticmagus · 2020-02-22T05:01:33Z

        ANativeWindow *window = ANativeWindow_fromSurface(env, surface);
-        runtime->UpdateWindow(width, height, window);
+        // TODO: Add UpdateWindow for Android.
+        // runtime->UpdateWindow(width, height, window);


Don't forget to add this functionality back!

syntheticmagus · 2020-02-22T05:01:47Z

        std::string url = env->GetStringUTFChars(sourceURL, &iscopy);
-        runtime->Eval(env->GetStringUTFChars(source, &iscopy), url);
+        std::string src = env->GetStringUTFChars(source, &iscopy);
+        // TODO: Put this on ScriptLoader?


Go ahead and do as part of this PR?

bghgary · 2020-02-25T00:25:50Z

        InputManager::Initialize(*runtime, *inputBuffer);

-        Babylon::ScriptLoader loader{ *runtime, rootUrl };
+        Babylon::ScriptLoader loader{ *runtime, runtime->RootUrl };


We chatted offline and it would be better as a getter.

bghgary · 2020-02-25T00:30:51Z


-    AppRuntime::~AppRuntime()
+    AppRuntime::AppRuntime(const char* rootUrl, std::unique_ptr<WorkQueue> workQueue)
+        : JsRuntime([& workQueue = *workQueue](auto func) { workQueue.Append(std::move(func)); })


nit: workQueue = workQueue.get()

bghgary · 2020-02-25T01:00:24Z

+        constexpr auto JS_ROOT_URL_NAME = "RootUrl";
+    }
+
+    void InitializeXMLHttpRequest(JsRuntime& runtime, const char* rootUrl)


Suggested change

void InitializeXMLHttpRequest(JsRuntime& runtime, const char* rootUrl)

void InitializeXMLHttpRequest(JsRuntime& runtime, std::string rootUrl)

syntheticmagus added 17 commits February 13, 2020 15:18

Well and truly down the rabbit hole now.

dce2cb3

Massive reshuffle, moved almost everything to be at least closer to w…

09b6f4d

…here we want it long-term.

More reshuffling, created CMakeLists for dependencies folder.

e32f443

With quite a bit commented out, able to build basic core using new st…

5407aee

…ructure!

First version of AppRuntime, constituting a MAJOR refactor of Runtime…

c6b5081

…Impl's responsibilities.

Remove pImpl pattern rendered unnecessary by the fact that all of App…

8f23024

…Runtime's hideable behaviors ended up getting consumed by the TaskChain.

Refactored again, split core into separable projects, enabled V8.

09a8d05

Able to successfully render, refresh, and close nothing.

83fdca6

Plugins!

df858e6

ScriptLoader and first proof-positive of JavaScript code executing.

b15e75c

Very minor cleanup.

3647e47

Now able to build NativeEngine again. Have to refactor it next.

7c62d35

Rendering the cube!

417f4ec

And XMLHttpRequest!

9303324

All Windows x64 builds working and tested: Win32/UWP, Chakra/v8.

1b06707

Deleted dead code.

472af9a

Validation tests to parity with master, as far as I can run them.

f8bf6ab

syntheticmagus requested review from CedricGuillemet, bghgary and ryantrem February 20, 2020 02:59

ryantrem reviewed Feb 20, 2020

View reviewed changes

bghgary reviewed Feb 20, 2020

View reviewed changes

syntheticmagus commented Feb 20, 2020

View reviewed changes

syntheticmagus and others added 7 commits February 20, 2020 12:51

Update Core/AppRuntime/CMakeLists.txt

9fabadd

Co-Authored-By: Gary Hsu <bghgary@users.noreply.github.com>

Changes for macOS.

b2cc8ca

Fixed XR for Windows after macOS changes.

7f369c1

Merge from master.

f2c8d78

First round of PR comments.

f38783c

Additional changes informed by errors from CI.

bf83331

Further CI-inspired changes for Android.

567af76

syntheticmagus added 9 commits February 21, 2020 11:34

Moar CI-inspired changes for cross-plat builds.

9b5695c

Ditto.

e3dcc67

Fixed UWP and added OpenGL to list of enabled graphics libraries for …

e1b68f8

…NativeEngine.

Added plugin dependencies for Android build.

78350c0

Binary dir modification for Android.

f7e64fb

I can see the matrix.

d4655b6

FINALLY adding changes to Android JNI integration.

9d07056

Added missing includes and switched from deleted type.

d0040e5

Fixed silly mistake.

02244f6

syntheticmagus marked this pull request as ready for review February 22, 2020 00:39

syntheticmagus added 3 commits February 21, 2020 17:18

Clang format.

cae0067

Introduced RunPlatformTier for AppRuntime.

08321e4

Protected contract changes to JsRuntime and comments to clarify.

f82497e

syntheticmagus commented Feb 22, 2020

View reviewed changes

syntheticmagus added 2 commits February 24, 2020 15:00

Fixed two TODOs for Android.

af27115

Fixed order of arguments error in Android.

fe1bf82

bghgary approved these changes Feb 25, 2020

View reviewed changes

syntheticmagus added 4 commits February 24, 2020 17:30

Final PR changes, not yet propagated to macOS, iOS, and Android.

207e29a

Removed mismatched paren.

deaa65f

Propagated final changes to macOS, iOS, and Android.

1511dec

Removed more auto-inserted parentheses from macOS and iOS files.

8e143c0

syntheticmagus merged commit 637c327 into BabylonJS:master Feb 25, 2020

syntheticmagus deleted the pluginRefactor branch February 25, 2020 02:15

This was referenced Mar 12, 2020

V8 teardown crash: order-of-destruction between JS context and Napi::Env #143

Closed

Refresh is broken on V8 #148

Closed

This was referenced Mar 12, 2020

Integration with React Native #133

Closed

Plugin Refactor Cleanup #175

Closed

syntheticmagus mentioned this pull request Mar 23, 2020

Order of destruction among inter-dependent N-API objects #142

Closed

	const std::string& GetRootUrl() const;
	const std::string& RootUrl() const;

	JsRuntime::JsRuntime(DispatchFunctionT&& dispatchFunction)
	JsRuntime::JsRuntime(DispatchFunctionT dispatchFunction)


		add_library(BabylonNativeUtils ${SOURCES})

		target_include_directories(BabylonNativeUtils PRIVATE "Include/Babylon")

	void InitializeXMLHttpRequest(JsRuntime& runtime, const char* rootUrl)
	void InitializeXMLHttpRequest(JsRuntime& runtime, std::string rootUrl)

Uh oh!

Conversation

syntheticmagus commented Feb 20, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syntheticmagus Feb 20, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bghgary left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syntheticmagus left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

syntheticmagus Feb 20, 2020 •

edited

Loading