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

Unity related changes #690

Draft
wants to merge 71 commits into
base: main
Choose a base branch
from
Draft

Unity related changes #690

wants to merge 71 commits into from

Conversation

atteneder
Copy link
Contributor

Hello dear Draco developers,

This PR mostly serves the purpose of suggesting additions/changes, kick off discussions and not primarily to be merged in the end.

The code can be tested in the related Unity package DracoUnity 2.0.0-preview.

Split up decode function

The decode function could be sped up for Unity (and maybe other systems as well) by splitting it into phases:

  1. Parse Draco data and provide final index and vertex buffer structure and size. Unity is then able to allocate resources properly ahead of time (which has to be done on the main thread)
  2. Perform actual decoding. This can now be done efficiently on threads, even in parallel (for each vertex attributes)

Encoding

I've created an additional library that allows encoding Unity meshes to draco data/files.

Others

  • GitHub action to build Unity libraries for all platforms in one go
  • Optional coordinate system conversion (required in glTF context)
  • Provide Unity package for easy installation/usage (see DracoUnity)

The latter is nothing new, but I'm open to re-integrate this package into the original repository. We could create a GitHub action that builds the native libaries, adds it to the package and deploys it to a special branch. From there it could be deployed to a package manage server (like verdaccio or OpenUPM)

Let me know your thoughts,
Thanks

@tomfinegan
Copy link
Contributor

@atteneder Thanks for the PR. I'm going to start by going over the GitHub actions and CMake stuff. Frank and l will look more deeply into the code over the next week or so-- things are pretty busy at present.

Re the GH actions stuff: there are some internal policy items that necessitate some changes to our repo configuration before we can accept CI automation that touches third-party actions. This shouldn't impact you much, but it's slowing the process down because Google projects can't accept the addition of third-party actions without these changes. As I currently understand the only change on your side is that CI integration must use specific released versions of third-party actions, but beyond that nothing is jumping out at me.

Copy link
Contributor

@tomfinegan tomfinegan left a comment

Choose a reason for hiding this comment

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

Sorry for the slow turnaround on this. I've really only gone through the CI and build related changes. Overall they look fine excepting the packaging related stuff, which I don't think we can accept in the parent repo.

.github/workflows/unity.yml Outdated Show resolved Hide resolved
.github/workflows/unity.yml Outdated Show resolved Hide resolved
.github/workflows/unity.yml Outdated Show resolved Hide resolved
.github/workflows/unity.yml Outdated Show resolved Hide resolved

linux:

runs-on: ubuntu-18.04
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use ubuntu-latest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've had a users report an issue when using a library built on ubuntu-latest (20.10 at the time) on ubuntu 18.04 (correct glibc version was not found).

Is there a better way to achieve backwards compatibility? Or cross distribution compatibility in general?

CMakeLists.txt Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
cmake/toolchains/i686-linux.cmake Outdated Show resolved Hide resolved
cmake/toolchains/i686-linux.cmake Outdated Show resolved Hide resolved
@atteneder
Copy link
Contributor Author

Thank you very much @tomfinegan for the excellent feedback and sorry I wasn't able to pick it back up for so long.

I tried to implement all suggestions and returned some questions where things are not clear to me. Depending on your answer I'll remove the packaging (or not), test if the CI works and make the PR ready for review.

@google-cla
Copy link

google-cla bot commented Sep 11, 2021

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and then comment @googlebot I fixed it.. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

…roceeds until all mesh and vertex attributes are known (This is required for clients to prepare resources like allocating buffers). Step two does the actual decoding of vertex attributes. This change allows for certain optimization in the Unity integration.

Note: WIP Only tested on MeshEdgebreakerDecoder.
… to left-hand (required for glTF embed Draco data)
atteneder and others added 9 commits April 26, 2023 12:12
…nded coordinate system in Unity encoder wrappers. Useful for Unity glTF export purpose.
* chore: Fixed CI runner image to avoid future surprises

* ci: Refactored/simplified Unity build script

* ci: Disabled Emscripten builds, as those moved into other CI jobs

* ci: Fixed deprecated parameter

* fix: Adressed linker errors due to duplicated symbols in `draco` and `dracodec_unity`.

- Consolidated dracodec_unity and dracoenc_unity into a single target named draco_unity

* chore: Renamed iOS simulator library name to avoid name conflict with device SDK variant.

* fix: Attempt to create universal binary

* fix: Attempt to create universal binary by providing architectures

* feat: Apple tvOS and Apple visionOS support

* chore: Raised iOS/tvOS target to 11.0, since this is the minimum version for Unity 2020 LTS

* feat: Added Windows ARM64 support

* fix: visionOS required dynamic libraries

* Revert "chore: Raised iOS/tvOS target to 11.0, since this is the minimum version for Unity 2020 LTS"

It broke the build

This reverts commit 1eb874b.

---------

Co-authored-by: Andreas Atteneder <[email protected]>
* feat: Initial version of the WebGL packages
* feat(ci): Yamato CI scripts
* feat: Unit test that sanity checks the native library file sizes
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