Skip to content

--[BE] Merge most habitat libraries into a single library to get rid of circular deps.#2511

Merged
jturner65 merged 16 commits intomainfrom
BE_MergeLibraries
Dec 4, 2024
Merged

--[BE] Merge most habitat libraries into a single library to get rid of circular deps.#2511
jturner65 merged 16 commits intomainfrom
BE_MergeLibraries

Conversation

@jturner65
Copy link
Copy Markdown
Contributor

@jturner65 jturner65 commented Dec 2, 2024

Motivation and Context

Habitat has long suffered from circular dependencies since every separate namespace was built as a separate library. Although having these individual libraries sped up compilation time for developers, it forced link multiplicity of up to 4 for certain libraries to link correctly.

This PR merges most of these separate libraries (except for bindings and gfx_batch) into a single library, habsim, that gets rid of the issues with circular dependencies. Developer compilation time will increase a little since the entire library will have to be rebuilt for any changes to any non-bindings/non-gfx_batch subsystem, but the benefits of no longer having circular dependencies.

This PR also removes all Plugins (except for PrimitiveImporter) from the main library and instead has shared libraries (i.e. bindings), applications and tests link to the ones they use.

Lastly, this PR introduces support for using Magnum::UfbxImporter for Obj and Fbx files, in an effort to diminish our reliance on Assimp. I think the only support we require from Assimp right now is ascii ply files (StanfordImporter handles binary plys).

How Has This Been Tested

Types of changes

Locally c++ tests pass

  • Docs change / refactoring / dependency upgrade
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have completed my CLA (see CONTRIBUTING)
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@jturner65 jturner65 requested review from 0mdc, aclegg3 and mosra December 2, 2024 21:08
@facebook-github-bot facebook-github-bot added the CLA Signed Do not delete this pull request or issue due to inactivity. label Dec 2, 2024
Copy link
Copy Markdown
Contributor

@aclegg3 aclegg3 left a comment

Choose a reason for hiding this comment

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

Thanks @jturner65 , given that most 2nd order users are installing from conda these days and full install isn't that much of a drag, this simplifying refactor seems like a good move.
We can break things back up later if we feel there is value.

@mosra
Copy link
Copy Markdown
Contributor

mosra commented Dec 3, 2024

Developer compilation time will increase a little since the entire library will have to be rebuilt for any changes to any non-bindings/non-gfx_batch subsystem

Just to clarify, there's no increase, as this happened before as well. In other words, there's no negative effect of this PR whatsoever, only ✨ positives ✨.

Before, if you modified something in the, say, assets library, everything depending on the assets library got relinked, thus also the massive LTO'ed bindings, and ultimately (most of) all unit tests due to the circular dependencies of everything depending on everything else. Now, if you modify something in the assets/ directory, executables and libraries depending on the habsim library gets relinked, thus basically the same set as before, i.e. everything.

Incremental building of just a single unit test is doable the same way as before, by running ninja SimTest for example, and the set of incrementally built files shouldn't change compared to before either.

As Alex says, if a certain part of the codebase gets iterated on a lot in isolation, the library can be broken up again. Which is already the case for gfx_batch, which is why it's kept separate.

Comment thread src/esp/CMakeLists.txt Outdated
Comment thread src/esp/CMakeLists.txt Outdated
Comment thread src/esp/CMakeLists.txt Outdated
Comment thread src/esp/CMakeLists.txt Outdated
Comment thread src/esp/CMakeLists.txt Outdated
Comment thread src/esp/CMakeLists.txt
Comment thread src/esp/CMakeLists.txt Outdated
endif() #BUILD_WITH_BULLET

# enable when finished? Slows compilation substantially
# set_directory_properties(PROPERTIES CORRADE_USE_PEDANTIC_FLAGS ON)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That fix we talked about is in mosra/corrade@280e6b6. But again, there's quite a few breaking changes in Magnum since the last update, so I'm not sure when it would be the best time to update here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can discuss the breaking changes in slack. TBH I would think it worthwhile to investigate updating Habitat to the most recent Magnum so any future Magnum growth will be more easily incorporated should it be useful, but of course that would depend on the scope of the changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What I wanted to say is that I unfortunately don't currently have the bandwidth to adapt to the changes. In C++ the changed APIs are just emitting deprecation warnings, so that's fine (you can upgrade gradually as necessary, for example), but for Python there's no such deprecation workflow in place, and so all the code needs to be adapted immediately.

For example there are no application mouse events anymore, it's a PointerEvent now, to have a single abstraction for a mouse, pen and touch. Again, in C++ the old MouseEvent interface is deprecated, but in Python it's just gone.

If you'd have time to do these changes, however, that's a way... :)

Comment thread src/esp/CMakeLists.txt Outdated
Copy link
Copy Markdown
Contributor

@0mdc 0mdc left a comment

Choose a reason for hiding this comment

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

Nice! Thanks for doing this!

@jturner65 jturner65 merged commit 8e83c12 into main Dec 4, 2024
@jturner65 jturner65 deleted the BE_MergeLibraries branch December 4, 2024 10:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed Do not delete this pull request or issue due to inactivity.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants