google-ar / arcore-unreal-sdk

ARCore SDK for Unreal
https://developers.google.com/ar/
Apache License 2.0
277 stars 122 forks source link

GoogleARCoreCameraInstrinsics.h is missing _API Macro #38

Closed error454 closed 5 years ago

error454 commented 5 years ago

Hey, the ue4 arcore repo doesn't have an issue tracker so I'm posting this here.

I went to use UGoogleARCoreCameraIntrinsics in C++ today and noticed that I was getting linker errors for all of the functions I was calling in UGoogleARCoreCameraIntrinsics. It looks like this class is missing the API macro, it should be: class GOOGLEARCOREBASE_API UGoogleARCoreCameraIntrinsics : public UObject vs class UGoogleARCoreCameraIntrinsics : public UObject

I fixed it up and things are working as expected. I took a look through the other public includes and this class seems to be the only one that snuck by without the macro. For the future, are you guys interested in taking PR's on your UE4 fork? This is a trivial change so I figured I'd just report it this time around.

bopangzz commented 5 years ago

Thanks for reporting this issue! We will get this fixed in our next release. We currently don't take PR's on our UE4 fork. If you have any suggestion or feature request, please create an issue here! Thank you!

error454 commented 5 years ago

Hey, one final addition to this that I just found while deploying. Including GoogleARCoreCameraInstrinsics.h means I also need to resolve arcore_c_api.h on build. This isn't added to any public include paths. Here is my diff for GoogleARCoreBase.Build.cs to resolve this.

+++ b/Engine/Plugins/Runtime/GoogleARCore/Source/GoogleARCoreBase/GoogleARCoreBase.Build.cs
@@ -16,6 +16,14 @@ namespace UnrealBuildTool.Rules
                                        "GoogleARCoreBase/Private",
                                }
                        );
+
+                       PublicIncludePaths.AddRange(
+                               new string[]
+                               {
+                                       Path.Combine(EngineDirectory, "Source/ThirdParty/GoogleARCore/include"), // For arcore_c_api.h when included by C++
+                               }
+                       );
+
                        PublicDependencyModuleNames.AddRange(
                                        new string[]
                                        {
bopangzz commented 5 years ago

Nice catch. We will resolve this in our next release as well. I think we shouldn't include "arcore_c_api.h" in that .h file.