Closed AtomicTroop closed 9 months ago
Hey @AtomicTroop ! First off -- thank you for taking the time to log this issue!
(sidenote: you are indeed right regarding the sidenote for PublicIncludePaths
, I'll remove it from the doc today)
The current main is indeed stable, at least on PC: I never had the chance to test on Apple hardware.
I'm waiting to finish the documentation write-up before locking the first tag in, as I keep on discovering a few minor issues/details here and there.
For the issue at hand, I'm a bit at a loss. I tried locally with a fresh project (both blueprint and C++) but I can't seem to reproduce the errors. I know the code is using some C++20 features which may not be your default (Unreal supports as low as C++17, but should come with C++20 as a default) on Mac?
The first error about FNonAbandonableTask
is puzzling because it's as if UnrealHeaderTool is missing some forward declaration in the code (or not taking a class ). I'd like to try and rule out something with you:
If you go to the source of the plugin, and edit PCGExMT.h at line 204 add an explicit forward declaration like so:
...
}
}
class FPCGExNonAbandonableTask; // <- This line
class PCGEXTENDEDTOOLKIT_API FPCGExAsyncManager
{
friend class FPCGExNonAbandonableTask; // <- this is there already and used to be enough on PC
public:
~FPCGExAsyncManager();
...
If this helps you get rid of the first error, the C++ environment/version you're using is somehow too strict.
If that seem to solve the issue, I'll do a cleanup pass based on your error log hoping that it solve the issues on the M1 but we're in for some nasty back'n forth; and even then there are some C++ template errors I'm not sure what the fix would be :/
Thank you for the quick response, I also suspected that modern C++ features were a possible culprit here, as I was also looking at the friend class declaration and wondering why it wasn't enough.
The forward declaration does work and prunes out a big chunk of the errors. I'll continue to look further into why the project isn't using C++20. It's totally possible that I have some global configuration left over from other projects on this laptop.
At the very least, the targets are on V4 build settings which should be using C++20.
A quick googling seems to say that each version of XCode comes with a set clang, and it looks like XCode version range available on M1 isn't C++20 -- tbh I've very much not familiar with Apple dev environment :/
Still, since Unreal 5.3 vanilla does support older versions, I'll need to tweak my sources to make sure this doesn't happen. Your log will be very helpful in that regard ;)
Good note on XCode, especially because I've been holding back on major updates as they always find a way to break my development setup one way or another. I'll have to sort that out sooner rather than later.
Regardless, as you said, it would definitely be good form for this plugin to have support at least down to C++17. You might be able to easily replicate the errors or otherwise test the support simply by dropping your project build settings version to V3, or potentially even V2 for support further back?
hmm so I did some tests compiling with CppStandard = CppStandardVersion.Cpp17;
in Build.cs as well as DefaultBuildSettings = BuildSettingsVersion.V4;
3
and 2
in Target.cs, and it highlighted a few issues which I fixed on the main. Mostly missing explicit constructors.
I couldn't get most of the warnings and errors you have in your log, so hopefully they're just collaterals of the underlying C++20 features I was loosely using.
If you have time to pull the latest, please let me know if this fixes it on your end 👀
Pulled your new changes just now, looks like most errors were cleared, the rest are all in PCGExGeoPrimtives.h
.
All the errors left are:
comparison between pointer and integer ('int' and 'int (*)()')
array index 2 is past the end of the array (which contains 2 elements) [-Werror,-Warray-bounds]
array index 3 is past the end of the array (which contains 3 elements) [-Werror,-Warray-bounds]
Each one appearing multiple times.
The first of the three seems to be fixed simply by explicitly calling the static Dimension
function
for (int i = 0; i < Dimension; i++) { sum += Position[i] * Position[i]; } // broken
for (int i = 0; i < Dimension(); i++) { sum += Position[i] * Position[i]; } // fixed
No idea on the other two yet, I'm surprised they're even caught by compile time analysis.
Edit:
Curiously, explicitly defining a static const Dimension in TFSimplex and replacing all references to the DIMENSIONS template int with the static const Dimension seems to fix all the array index errors mentioned in my previous message:
// in PCGExGeoPrimitives.h, line 103 and onwards
template <int DIMENSIONS>
class PCGEXTENDEDTOOLKIT_API TFSimplex
{
static const int Dimension = DIMENSIONS;
public:
double Normal[Dimension];
double Centroid[Dimension];
...
TFVtx<DIMENSIONS>* Vertices[Dimension];
...
...
This however brought up some new errors:
0>PCGExSubPointsOrientAverage.h(19,22): Error : inline function 'UPCGExSubPointsOrientAverage::Orient' is not defined [-Werror,-Wundefined-inline]
0> inline virtual void Orient(FPCGPoint& Point, const FPCGPoint& PreviousPoint, const FPCGPoint& NextPoint) const override;
0> ^
0>PCGExSubPointsOrientAverage.gen.cpp(79,32): Reference : used here
0> UPCGExSubPointsOrientAverage::UPCGExSubPointsOrientAverage(const FObjectInitializer& ObjectInitializer) : Super(ObjectInitializer) {}
0>
0>PCGExSubPointsOrientOperation.h(29,22): Error : inline function 'UPCGExSubPointsOrientOperation::Orient' is not defined [-Werror,-Wundefined-inline]
0> inline virtual void Orient(FPCGPoint& Point, const FPCGPoint& PreviousPoint, const FPCGPoint& NextPoint) const;
0> ^
0>PCGExSubPointsOrientLookAt.gen.cpp(161,31): Reference : used here
0> UPCGExSubPointsOrientLookAt::UPCGExSubPointsOrientLookAt(const FObjectInitializer& ObjectInitializer) : Super(ObjectInitializer) {}
0>
0>PCGExSubPointsOrientWeighted.h(24,22): Error : inline function 'UPCGExSubPointsOrientWeighted::Orient' is not defined [-Werror,-Wundefined-inline]
0> inline virtual void Orient(FPCGPoint& Point, const FPCGPoint& PreviousPoint, const FPCGPoint& NextPoint) const override;
0> ^
0>PCGExSubPointsOrientWeighted.gen.cpp(101,33): Reference : used here
0> UPCGExSubPointsOrientWeighted::UPCGExSubPointsOrientWeighted(const FObjectInitializer& ObjectInitializer) : Super(ObjectInitializer) {}
0>
0>PCGExSubPointsOrientWeighted.h(25,22): Error : inline function 'UPCGExSubPointsOrientWeighted::OrientInvertedWeight' is not defined [-Werror,-Wundefined-inline]
0> inline virtual void OrientInvertedWeight(FPCGPoint& Point, const FPCGPoint& PreviousPoint, const FPCGPoint& NextPoint) const;
0> ^
0>PCGExSubPointsOrientWeighted.gen.cpp(101,33): Reference : used here
0> UPCGExSubPointsOrientWeighted::UPCGExSubPointsOrientWeighted(const FObjectInitializer& ObjectInitializer) : Super(ObjectInitializer) {}
0>
I managed to clear these errors simply by removing the inline keywords, although you probably have a better idea of the performance ramifications if these functions are not inlined.
After these modifications, I can finally compile and run the plugin. I'll report back in later after I've tested it out properly to confirm everything is working as expected.
Thanks for the detailed update!
I slightly refactored the code in order to avoid using an internal static int
and keep on relying on the template value (since the DIMENSIONS
template is propagated from different wrappers). This was a nice fix nonetheless; I'll fallback to that if the update keeps on throwing on your end.
The reason why this was throwing initially is because some functions (GetV3
) was indeed referencing indices larger than the DIMENSIONS
of the template, but behind a check (so runtime safe). Not sure why the compiler never complained on my end.
I switched to templated static function instead of embedded members, which should be enough; and I suspect that what it was doing under the hood instead of throwing.
As for the inline, looking at the error I'm assuming it simply doesn't like that the inlined implementation is actually in a separate .cpp instead of actually inlined in the header file. (which, duh) The impact of removing inlining is probably negligible given PCG is not designed for runtime execution anyway, and Orient is not the worst offender perf-wise in the library :D
Bottom line: I pushed an update, which hopefully fixes things for good! Again, thanks A LOT for taking the time to report all this ♥
I removed my local fixes and tested out yours, looks like they're compiling fine on my end. I think at this point it's safe to close this issue as resolved. I'd guess that the reason these issues only appeared on my end is again because of clang's default configuration, although I'd expect these kinds of things to only come up if I were running the clang static analyzer that's currently in beta for Unreal.
Regarding your remark about PCG not being designed for runtime execution, it's worth noting that executing in runtime context is one of the main focus points listed on the Unreal Engine roadmap when it comes to the future of the PCG plugin. It's also one of the main usecases I'm interested in, so I'm hoping to see some new features on that front in 5.4 already.
On a separate note, thank you for the recent improvements to the documentation. I'm having a much easier time playing with these nodes with the info you've added. :)
When using runtime I had "executing every frame" in mind. A lot (not all tho) of the nodes in this lib are running async and I'm trying to stay away from the main thread as much as I can.
Parallelization comes with a lot of issues when it comes to determinism, so some nodes are slower than they initially were but still async. Working with graphs is very expensive for now, but there's a lot of room for optimization as well -- you can safely use it to generate stuff at runtime, but given the processing cost of certain nodes, I wouldn't update the PCGs every frame.
As for the documentation : it is my main focus at the moment, so I keep adding new content on a daily basis.
Also, feel free to push a dummy PR on this repo if you want your profile marked as a contributor: the above wouldn't be fixed if it wasn't for your debugging effort ;)
Fair point on "runtime" semantics. Regarding performance, I had a similar node based PCG tool as a pet project but for Unity, where my approach to performance was running the node computations on the GPU in spatial chunks and passing a 3D buffer down the node dependency chain in reverse to keep memory usage under control. It of course came with some heavy limitations but the intended purpose of the tool was much more focused in the first place.
I won't go further into the topic here as this is veering off topic for the (resolved) issue, but GPU accelerated PCG nodes are an area I intend to experiment with as soon as that feature makes it to UE (also on roadmap, conveniently).
As for the PR, I appreciate the offer but I'll hold off on it for now in hopes that I can contribute something more tangible after I get more familiar with this plugin and its internals.
Hi, I'm testing out this plugin in a new, freshly created Unreal 5.3.2 C++ project. I'm developing on an M1 Macbook Pro, so this is an arm64 editor build.
I've added the plugin as a git submodule and added the C# side ModuleRules additions to my project's Build.cs.
As a side note, the
PublicIncludePaths
addition in the installation instructions seems to be unnecessary in 5.3 as per the comment left on that field's API docs:When I try to build the plugin after these installation steps, I get a big bunch of compilation errors, I've attached the logs at the end of this issue description.
I'm not quite sure what's going on here, whether it's something wrong with my local configuration or an issue with Mac/arm64 support maybe, but I'd also like to rule out the option that the current state of the main branch is broken, considering there's no stable tag to pin the submodule at right now. Any thoughts or advice would be appreciated.
Compile logs
<projectroot>
and<engineroot>
are the absolute paths to my local project and installed engine, respectively