Early C++ implementation for the CompoundClouds' system #5606
Conversation
|
We are currently in feature freeze until the next release. |
| return true; | ||
| public override bool CanConvert(Type objectType) | ||
| { | ||
| // Since CompoundCloudPlane is not a C# type, we return false |
There was a problem hiding this comment.
How is the data saved then if this converter no longer does it? Because this was the way if I recall correctly how saving and loading didn't clear out all the compound clouds.
There was a problem hiding this comment.
This can be taken care of later, but I'll note that this file doesn't follow our naming conventions: our C++ code should use the same naming conventions as our C# code (so no snake_case naming of variables or methods).
| void CompoundCloudPlane::_bind_methods() { | ||
| using namespace godot; | ||
|
|
||
| ClassDB::bind_method(D_METHOD("get_native_instance"), &CompoundCloudPlane::get_native_instance); |
There was a problem hiding this comment.
But when exposed through Godot the godot naming convention should be used. So this line should be:
| ClassDB::bind_method(D_METHOD("get_native_instance"), &CompoundCloudPlane::get_native_instance); | |
| ClassDB::bind_method(D_METHOD("get_native_instance"), &CompoundCloudPlane::GetNativeInstance); | |
| size = 104; | ||
|
|
||
| if (size <= 0) { | ||
| godot::UtilityFunctions::printerr("Invalid size for CompoundCloudPlane"); |
There was a problem hiding this comment.
The helper error macro should be used instead as that will automatically include the method name and line number the error happens so that's much more helpful way to print errors.
| density[x][y][index] += new_density; | ||
|
|
||
| texture_needs_update = true; | ||
| set_process(true); |
There was a problem hiding this comment.
Have you tested the performance impact of constantly calling the enable processing? Most clouds are basically never empty meaning that I think the overhead from enabling / disabling processing is probably more than what is gained by disabling the processing. I think this probably artificially inflates the benchmark numbers. If required I can modify the benchmark to use all compound cloud types in sequence to ensure fairness against the C# implementation that way.
|
|
||
| void CompoundCloudPlane::update_compound_colors() { | ||
| // Define a map of compound IDs to their corresponding colors from compounds.json | ||
| std::unordered_map<int, godot::Color> compound_color_map = { |
There was a problem hiding this comment.
Is this temporary code or? If this is not temporary this definitely needs to be sent by the C# side to configure the cloud correctly.
| } | ||
| catch (Exception e) | ||
| { | ||
| GD.PrintErr($"Error in CompoundCloudSystem._Process: {e.Message}\n{e.StackTrace}"); |
There was a problem hiding this comment.
I'm pretty sure there's no way anything here would cause an exception... because Godot Call will just print an error and not throw anything.
|
|
||
| private int GetCompoundID(CompoundDefinition compoundDefinition) | ||
| { | ||
| return (int)compoundDefinition.ID; |
There was a problem hiding this comment.
A lot of memory I think could be saved by using the underlying type of the ID, which I specifically chose to be uint16 as that gives still plenty of compound types but requires only half as much memory as an int.
| /// </summary> | ||
| /// <param name="compound">The compound type to take</param> | ||
| /// <param name="worldPosition">World position to take from</param> | ||
| /// <param name="fraction">The fraction of compound to take. Should be <= 1</param> | ||
| public float TakeCompound(Compound compound, Vector3 worldPosition, float fraction) |
There was a problem hiding this comment.
I think this method needs to be re-thought out as this is called on many points for a circle for a single absorb operation, meaning that there's a ton of calls going between C# and C++ when absorbing. Also why did you remove the first part of the summary? And also you seemed to add a bunch of locks in the C++ code so it is less parallel than this C# code. You should instead consider trying to use atomic reads and writes in C++.
| var compounds = (int[])cloudInstance.Call("get_compounds"); | ||
|
|
||
| if (!Array.Exists(compounds, id => id == compoundID)) |
There was a problem hiding this comment.
This allocates a bit of memory, would be really nice if a core operation like this didn't need to constantly allocate memory.
| continue; | ||
|
|
||
| // Then just need to check that it is within the cloud simulation array | ||
| if (x < cloud.Size && y < cloud.Size) | ||
| float amount = (float)cloudInstance.Call("amount_available", compoundID, x, y, 1.0f); |
There was a problem hiding this comment.
This is in a very big loop calling a ton of methods through the Godot calling interface.
There was a problem hiding this comment.
In general I think the goal with the conversion of the clouds to C++ would be that the visuals or functionality don't change in any significant way, which from your screenshots doesn't seem to be the case, but only performance would increase... Also out of the code architecture changes you've done many seem not necessary (and I didn't find out why you needed to add a global list of existing cloud planes) and when thinking about this conversion many architecture changes I anticipated needing to be done seem to not have been done (like converting the absorb / emit compound methods to an approach that reduces the number of calls between C++ and C#). So I have quite a lot of architecture feedback, some of which I specifically pointed out but there's so many instances that I couldn't comment on all of them.
There was a problem hiding this comment.
It looks like to me that you have done a ton of unnecessary changes to this file that make things a lot harder (so unrelated to just having the cloud types now be implemented in C++) and including removing documentation on methods. At the same time it looks like the methods I really think should be entirely rewritten for the new approach to reduce the C#<->C++ call count, are not entirely rewritten. So I sadly need to say that I think you've kind of done the opposite here than what I think should have been done.
This PR introduces a basic implementation of the CompoundClouds system in C++. While it lays the groundwork for future development, there are several known issues and limitations that need to be addressed. Once these issues are resolved, this PR will close Issue #4917.
Related Issues:
This implementation serves as a starting point, and I look forward to refining it further with your feedback.
Benchmark Results
I'd also like to share benchmark results comparing the CS benchmark with the C++ implementation:
Benchmark results for CloudBenchmark CS
Benchmark results for CloudBenchmark C++
Here's the visual results of my implentation:
