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.

Sign up for GitHub

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

Jump to bottom

Asset Importer Now Builds Inside of SDKWrapper Module #18273

Open
wants to merge 18 commits into
base: development
Choose a base branch
from
Open

Asset Importer Now Builds Inside of SDKWrapper Module #18273

wants to merge 18 commits into from
+96 −10

Conversation

AMZN-Gene
Copy link
Contributor

@AMZN-Gene AMZN-Gene commented Aug 27, 2024 •
edited
Loading

Upgrade to latest Assimp for Universal Scene Description (USD) support
image

  • Removing AssImp from 3rd Party and instead built by SDKWrapper using CMake's FetchContent.

    • Pulling Assimp changes will be easier now that it doesn't require going through the 3rdParty pipeline

  • Assimp library is a child folder of the SDKWrapper Module
    image

  • Adds ~2 minutes to build time

Resolves RFC (Part 1): o3de/sig-simulation#92

Tested
Tested by importing a single USD (no references) into editor using a build from source and SDK

@AMZN-Gene AMZN-Gene mentioned this pull request Aug 29, 2024
Open
@AMZN-Gene
Copy link
Contributor Author

WiP. Right now assimp is built under the root build folder. Still need to move to be under SDKWrapper in a "External" folder

@nick-l-o3de
Copy link
Contributor

Very nice, main concern I have is when someone uses it in an installer, and I'm not actually sure what should happen. I'm guessing all our use of assimp is as a static lib, and thus, its embedded in any dlls we ship, but its probably worth checking the linkage isn't broken or it doesn't need to install or copy assimp libs somewhere in the installer as part of this work. Normally you'd download the installer, and then it would fetch the assimp 3p from prebuilt.

@AMZN-Gene
Copy link
Contributor Author

Very nice, main concern I have is when someone uses it in an installer, and I'm not actually sure what should happen. I'm guessing all our use of assimp is as a static lib, and thus, its embedded in any dlls we ship, but its probably worth checking the linkage isn't broken or it doesn't need to install or copy assimp libs somewhere in the installer as part of this work. Normally you'd download the installer, and then it would fetch the assimp 3p from prebuilt.

Recast also uses FetchContent, this code block here mentions installer setup
https://github.com/o3de/o3de/blob/development/Gems/RecastNavigation/Code/CMakeLists.txt#L73-L93

@byrcolin byrcolin added the needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. label Sep 3, 2024
@AMZN-Gene
Merging dev latest
1036ba4 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
Moving assimp so it's under the SDKWrapper project instead of the root
ef6a1ae 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
Removing ios and android files from assimp builder since ios and andr…
7a1835f 
…oid dont ever build tools

Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
Merge dev latest
d5b8049 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
Stop assimp from adding debug postfix 'd' to debug module output names
700d361 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
remove unused variable in unittest.cmake
c243f53 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
Removing use of auto() for creating mock_method
12755c9 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene
Merge maestro fix
dc8dfee 
Signed-off-by: AMZN-Gene <[email protected]>
@AMZN-Gene AMZN-Gene marked this pull request as ready for review September 20, 2024 23:33
@AMZN-Gene AMZN-Gene requested review from a team as code owners September 20, 2024 23:33
nick-l-o3de
nick-l-o3de reviewed Mar 3, 2025
View reviewed changes
Copy link
Contributor

@nick-l-o3de nick-l-o3de left a comment

Choose a reason for hiding this comment

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

I like the idea of moving this into here, but we're currently looking at clean ways to do it. Please hold onto this, or get in touch, I think I can help clean this up in a way that will work very well with the installer version too

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nick-l-o3de nick-l-o3de nick-l-o3de left review comments

@spham-amzn spham-amzn spham-amzn approved these changes

Assignees
No one assigned
Labels
needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@AMZN-Gene @nick-l-o3de @spham-amzn @byrcolin