Skip to content

Conversation

@josefalanga
Copy link
Contributor

It's not wrapped in preprocessor directives because it's only called manually, so it shouldn't affect performance for projects not using it.

The intention of this is making a deep copy, that contains no references to the original in any way. If it could be done faster or there is some state still being referenced instead of copied that I'm missing, I'm accepting feedback.

@josefalanga josefalanga changed the title Implement World.Copy [Draft] Implement World.Copy Aug 28, 2025
@josefalanga josefalanga marked this pull request as ready for review September 2, 2025 21:52
@josefalanga josefalanga changed the title [Draft] Implement World.Copy Implement World.Copy Sep 2, 2025
@genaray
Copy link
Owner

genaray commented Sep 2, 2025

Thanks!
Could you add an Test for this? :)

And whats the difference to:
#241 ?

@josefalanga
Copy link
Contributor Author

josefalanga commented Sep 3, 2025

And whats the difference to: #241 ?

It seems that PR is intended to duplicate certain entities within the same world, resulting in additional entities. This PR, on the other hand, aims to create a copy of the entire world, resulting in the same world without extra entities.

Do you also want me to add a benchmark, like the other PR includes?

@josefalanga
Copy link
Contributor Author

josefalanga commented Sep 3, 2025

Thanks to the tests I found out Copy was destructive for the source Archetype. Given this is supposed to happen and Copy usages are widespread, I opted for adding an optional parameter to indicate when we don't want to do that, so the world Copy is not destructive for the source.

@josefalanga
Copy link
Contributor Author

josefalanga commented Sep 4, 2025

Ran some small benchmarks, skipping empty archetypes is a significant improvement it seems.

|       Method | Amount |      Mean |    Error |   StdDev |    Gen0 |    Gen1 | Allocated |
|------------- |------- |----------:|---------:|---------:|--------:|--------:|----------:|
|         Copy |    500 | 114.64 us | 1.987 us | 1.858 us |  9.5215 |  9.2773 | 159.05 KB |
| CopySkipping |    500 |  72.09 us | 1.016 us | 0.950 us |  6.1035 |  5.9814 |  99.93 KB |
|         Copy |   1000 | 114.83 us | 1.030 us | 0.964 us |  9.5215 |  9.2773 | 159.05 KB |
| CopySkipping |   1000 |  72.47 us | 0.940 us | 0.880 us |  6.1035 |  5.9814 |  99.93 KB |
|         Copy |   5000 | 135.64 us | 1.080 us | 0.843 us | 11.4746 | 11.2305 | 191.21 KB |
| CopySkipping |   5000 |  92.35 us | 1.191 us | 0.930 us |  8.0566 |  7.9346 | 132.09 KB |

This was done for 5 archetypes, when one of them is empty. If you see value on this, I could make them part of the PR.

@genaray genaray merged commit f2a5a71 into genaray:master Sep 18, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants