Add platform polyfills for retrieving window.deviceScalingRatio #582
Conversation
…/BabylonNative into add-engine-hardware-scaling-level
…/BabylonNative into add-engine-hardware-scaling-level
Co-authored-by: Ryan Tremblay <ryantrem@msn.com>
…d windows window header to CMAKElists, update applications to use new contract
…-scaling-ratio-polyfill
| void* windowPtr = (__bridge void*)engineView; | ||
|
|
||
| graphics = Babylon::Graphics::CreateGraphics(windowPtr, static_cast<size_t>(600), static_cast<size_t>(400)); | ||
| GraphicsConfiguration graphicsConfig = GraphicsConfiguration(); | ||
| graphicsConfig.windowPtr = engineView; |
There was a problem hiding this comment.
How does this compile? Isn't graphicsConfig.windowPtr of type MTKView*?
There was a problem hiding this comment.
we're declaring the interface EngineView to be an MTKView via L18
Core/Graphics/Include/UWP/CoreWindow/Babylon/GraphicsPlatform.h
Outdated
Show resolved
Hide resolved
| template<typename WindowT> | ||
| WindowT GetNativeWindow(); |
There was a problem hiding this comment.
If I understand correctly, this is the change that tries to eliminate the need to compile with the objective C++ flag, is that right? It seems very awkward though, and like we have a weird mix of known and sort of unknown types in the API.
There was a problem hiding this comment.
My understanding is that if we want this function as a member of GraphicsImpl we either A) go back to using void*, B) use the platform type in this header, causing compilation issues with our consumers if the headers arent c++, or c) use some templating to obscure the type in the header.
The alternative is to make it some odd utility function that uses GraphicsImpl? C++ makes these types of situations difficult.
There was a problem hiding this comment.
I agree that it's a little awkward, but (in my opinion) much less so than having a bunch of C++ libraries that have to be compiled as Objective C++ just so that they can find a type that they don't actually use in C++. The reason for templating this instead of forward-declaring MTKView is because Apple defines MTKView as an interface, which didn't seem to play nicely with our attempts to forward-declare it in C++. Because this is an internal contract, not a proper outward-facing API, I don't think it's too big a deal for it to have this small degree of awkwardness.
If it's bothersome, though, I can think of two other option to do this cleanly, though the first is (in my opinion) even more awkward: have a special platform-specific struct defined just like the config struct, the only purpose of which is to contain an MTKView*, then forward-declare that and use it as the return type. This is awkward because it entails having an entire type which has to be implemented per-platform just for the purpose of avoiding a templated function.
The second approach would be to simply move this function definition to a platform-specific header, using our CMake practices to deliver the right header to the right consumer. This is probably my favorite of the three, but it's still a little awkward because you'd need to take it out of the class, making it GetNativeWindowFromGraphicsImpl(GraphicsImpl&) or something, which could get odd. Bottom line, I think all three of these approaches are equally viable and equally awkward, so I don't really have a strong personal preference among them.
|
|
||
| namespace Babylon | ||
| { | ||
| class Graphics::Impl : public GraphicsImpl { }; |
There was a problem hiding this comment.
GraphicsImpl.h is now no longer included by Graphics.h,
So we declare Graphics::Impl in Graphics.h, then define it as our internal type in the .cpp where we use it.
The idea was to remove it from the public contract.
There was a problem hiding this comment.
Until I realized that we also needed to remove references to our platform types for consumers of the private contract as well, so the intent of this change was wrong. I don't really think its a bad change, however.
There was a problem hiding this comment.
This change also allows Graphics::Impl to become a private class, right? It's public now, though GitHub won't let me put a comment directly on it in the header because it's too far away from a changed line.
| @@ -126,7 +131,11 @@ extern "C" | |||
| { | |||
| ANativeWindow *window = ANativeWindow_fromSurface(env, surface); | |||
| g_runtime->Dispatch([window, width = static_cast<size_t>(width), height = static_cast<size_t>(height)](auto env) { | |||
| g_graphics->UpdateWindow<void*>(window); | |||
| GraphicsConfiguration graphicsConfig = GraphicsConfiguration(); | |||
There was a problem hiding this comment.
Same as before. You might consider giving GraphicsConfiguration a constructor that takes arguments, though, so you don't have to write out a block of code every time you make one of these.
There was a problem hiding this comment.
I figured using the default constructor and populating the fields independently in our sample playground gives us the flexibility to expand the GraphicsConfiguration object without needing to update existing code that populates this struct. I considered using a list initializer as well, and using a constructor with optional parameters, however we're still constrained by the ordering of the members in the struct, but maybe i'm playing it up to be more important than it is...
There was a problem hiding this comment.
The suggestion to have a constructor take arguments isn't really more than a nit, so it's not that important. That said, you can still choose to not use the constructor if it doesn't fit your use case, and you can have multiple constructors if you have several typical use cases. In this case, since you're writing the same four or five lines of code multiple times in multiple places, this initialization scenario seems common enough to warrant a constructor (which, if the struct is later expanded with other members, could just leave those blank if it wants to since they by definition won't pertain to this usage). As I said, though, it's a nit, so just a judgement call. The main thing this comment was to document was the same thing as the long one: the initialization code.
…window-device-scaling-ratio-polyfill
…window-device-scaling-ratio-polyfill
|
Hey @Drigax - since this has Babylon Native API breaking changes, are you planning to update Babylon React Native? There are some other PRs out with updates to both Babylon Native and Babylon React Native, and I think these are blocked until Babylon React Native is updated to deal with the API changes. |
I see, I don't believe the PRs are blocked, since they're updating the submodule to a commit prior to mine, however I have the changes staging here, and will have a PR open shortly: https://github.com/Drigax/BabylonReactNative/tree/update-graphics-contract |
| @@ -6,19 +6,18 @@ | |||
|
|
|||
| namespace Babylon | |||
| { | |||
| struct GraphicsConfiguration; | |||
There was a problem hiding this comment.
nit: This isn't where I was thinking the forward declaration for this would go; I was assuming you'd just include GraphicsPlatform.h here because (I think) the general expectation is that consumers of this file will need GraphicsPlatform.h. The forward declaration would then just be for GraphicsImpl.h, which is (I think) the only file where we explicitly need to avoid the platform types.
| @@ -0,0 +1,13 @@ | |||
| #pragma once | |||
|
|
|||
| #include <Babylon/Graphics.h> | |||
There was a problem hiding this comment.
I might be missing something, but this relationship seems backwards. Should Graphics.h include GraphicsImpl.h, not the other way around? I'm thinking that consumers should be including Graphics.h (which, will then give them GraphicsPlatform.h as an implementation detail); what's the reasoning for doing it this other way?
| @@ -6,19 +6,18 @@ | |||
|
|
|||
| namespace Babylon | |||
| { | |||
| struct GraphicsConfiguration; | |||
|
|
|||
| class Graphics | |||
| { | |||
| public: | |||
| class Impl; | |||
There was a problem hiding this comment.
I think this type can be private now.
| #include <Babylon/GraphicsPlatform.h> | ||
| #include "../GraphicsImpl.h" | ||
|
|
||
| constexpr float MILLIMETERS_TO_INCHES = 0.03937; |
There was a problem hiding this comment.
I think that number on the right is a double, not a float, but I could be wrong as I'm kind of surprised this compiled. Should this number use the f suffix?
| @@ -3,6 +3,7 @@ | |||
| #include <arcana/containers/ticketed_collection.h> | |||
|
|
|||
| #include <Babylon/JsRuntime.h> | |||
| #include <Babylon/Graphics.h> | |||
There was a problem hiding this comment.
Why Graphics.h? I didn't think this file needed to know about the Graphics contract, just the GraphicsImpl and GraphicsConfigration contracts. Do we ever deal with Graphics directly here?
| #include <basen.hpp> | ||
| #include <chrono> | ||
| #include <iterator> | ||
| #include "Window.h" |
There was a problem hiding this comment.
Conventionally, this goes at the top. What was the reasoning for moving this?
This PR modifies the Window polyfill contract to retain a reference to the native window implementation, and creates platform implementations of retreiving the DPI scaling of that window.
Dependent on Babylon.js pull request: BabylonJS/Babylon.js#9892 in order to enable auto-scaling on engine creation.
Fixes #482 #620