-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
O3DE engine Gem rename support #17059
O3DE engine Gem rename support #17059
Conversation
2ef5a23
to
d1fc67d
Compare
The following changes have been made to all of the Gems that come with the O3DE engine source code to make it easier to rename a Gem. Updated all of the Gems CMakeLists.txt to use the "${gem_name}" variable when referencing the Gem in CMake Targets. This allows the name of the CMake targets to update whenever the "gem_name" in the gem.json is changed. Updated all of the `AZ_DECLARE_MODULE_CLASS` macro invocations to reference the `O3DE_GEM_NAME` preprocessor token which is use to substitute the "gem_name" in the gem.json for the extern "C" `CreateModuleClass_Gem_*` function which the launcher_generator.cmake invokes in Monolithic builds to create an AZ::Module derived class instance for each active gem. resolves o3de#17034 Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
d1fc67d
to
656ec50
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this, just some minor suggestions and a question so I understand why we need to support the case where O3DE_GEM_NAME
isn't defined.
All the comments that started with "DO NOT MODIFY THIS LINE UNLESS YOU RENAME THE GEM" have been removed. The externed "C" function that instantiates an AZ::Module derived class in monolithic build now automatically adjust account for a rename of the Gem. Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing! Thanks for making that macro, makes it so much more clean!
The repeat boilerplate logic in each Gem has been replaced with a CMake macro called "o3de_gem_setup" which uses the directory of the calling CMakeLists.txt file to locate the nearest ancestor gem.json file. It uses that directory of that gem.json file as the `${gem_path}` value. It reads the "gem_name" field from that gem.json and sets that as the `${gem_name}` variable. It also reads the "version" field from that gem.json and sets that as the `${gem_version}` variable. Finally it sets the `${pal_dir}` variable based to the equivalent `<CMakeLists.txt directory>/Platform/<configured platform>/` directory using the directory of the calling CMakeLists.txt and the current platform that CMake is being configured for(Windows, Linux, Android, MacOS, iOS, or a "restricted platform"). Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
99603bb
to
40f48d9
Compare
function to also output the name of the Allocator when the Assert Triggers Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
d662c85
to
5e57b7a
Compare
returning a different value for the amount of bytes deallocated, than were allocated. The issue is that when the HpAllocator is used for aligned allocation of >=32 bytes, it could find a block that could fulfill the allocation request, but wasn't aligned. If the block need to be adjusted for a size that was >0 but less than the size of block header(16 bytes) + the size of the free node(40 bytes), than instead of being to split the block into two blocks where the first block would be a free block that gets added to the free list and the second block would be the actual aligned block, the code would try to shift the previous block to be large enough such that the next block would start aligned on the specified alignent using the `shift_block` function. This would result in previous block storing a larger size value. So when the memory associated with the block was deallocated, it would report a size larger than the size reported when it was allocated. This fixes an issue where the Tracking the number of allocated bytes in the ChildAllocatorSchema and SimpleAllocatorSchema would result in a non-zero value after all of the allocations have been deleted. This because the `ChildAllocatorSchema::allocate` function relies on amount of bytes returned from the actual child allocator `allocate()` call to be matched when the pointer that was allocated is deallocated via the `ChildAllocatorSchema::deallocate` function which uses the child allocator `deallocate()` call. ``` 20:28:39 [----------] 1 test from MotionSetFixture 20:28:39 [ RUN ] MotionSetFixture.MeshLoadTest 20:28:39 D:\workspace\o3de\Code\Framework\AzCore\AzCore/Memory/ChildAllocatorSchema.h(155): error: Child Allocator "EMotionFXAllocator": Total allocated bytes is less than zero with a value of -16. Was deallocate() invoked with an address that is not associated with this allocator? This should never occur 20:28:39 D:\workspace\o3de\Code\Framework\AzCore\AzCore/UnitTest/TestTypes.h(76): error: Expected equality of these values: 20:28:39 sizeBeforeTestRan 20:28:39 Which is: 0 20:28:39 sizeAfterTestRan 20:28:39 Which is: 18446744073709551600 20:28:39 for allocator EMotionFXAllocator with 0 allocation records ``` Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
Co-authored-by: Steve Pham <82231385+spham-amzn@users.noreply.github.com> Signed-off-by: lumberyard-employee-dm <56135373+lumberyard-employee-dm@users.noreply.github.com>
{ | ||
const size_t sizeWithBlockHeaders = size + 3 * sizeof(block_header); // two fences plus one fake | ||
|
||
// tree_system_alloc() will return an offset that is a multiple of OS_VIRTUAL_PAGE_SIZE which is 64KiB |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if its better to put these longer explainations and details in a mark down file in this folder (something like HphaAllocator.md) and then just reference that file in the code instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That could be arranged.
The following changes have been made to all of the Gems that come with the O3DE engine source code to make it easier to rename a Gem.
Updated all of the Gems CMakeLists.txt to use the "${gem_name}" variable when referencing the Gem in CMake Targets.
This allows the name of the CMake targets to update whenever the "gem_name" in the gem.json is changed.
Updated all of the
AZ_DECLARE_MODULE_CLASS
macro invocations to reference theO3DE_GEM_NAME
preprocessor token which is use to substitute the "gem_name" in the gem.json for the extern "C"CreateModuleClass_Gem_*
function which the launcher_generator.cmake invokes in Monolithic builds to create an AZ::Module derived class instance for each active gem.resolves #17034
How was this PR tested?
Built Monolithic and Non-Monolithic SDK as well as built the AutomatedTesting project in Monolithic and non-Monolithic