Skip to content
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

Merged

Conversation

lemonade-dm
Copy link
Contributor

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 #17034

How was this PR tested?

Built Monolithic and Non-Monolithic SDK as well as built the AutomatedTesting project in Monolithic and non-Monolithic

@lemonade-dm lemonade-dm requested review from a team as code owners November 14, 2023 01:46
@lemonade-dm lemonade-dm force-pushed the engine-gem-renaming-support branch 2 times, most recently from 2ef5a23 to d1fc67d Compare November 14, 2023 02:18
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>
@lemonade-dm lemonade-dm force-pushed the engine-gem-renaming-support branch from d1fc67d to 656ec50 Compare November 14, 2023 03:36
Copy link
Contributor

@AMZN-alexpete AMZN-alexpete left a 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>
Copy link
Contributor

@AMZN-alexpete AMZN-alexpete left a 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>
@lemonade-dm lemonade-dm force-pushed the engine-gem-renaming-support branch from 99603bb to 40f48d9 Compare November 15, 2023 01:55
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>
@lemonade-dm lemonade-dm force-pushed the engine-gem-renaming-support branch from d662c85 to 5e57b7a Compare November 16, 2023 23:53
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
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could be arranged.

@lemonade-dm lemonade-dm merged commit 324c031 into o3de:development Nov 18, 2023
@lemonade-dm lemonade-dm deleted the engine-gem-renaming-support branch November 18, 2023 22:10
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.

Renamed gem name in gem.json has linker errors in monolithic release builds
5 participants