search

atteneder / DracoUnity

Draco 3D Data Compression Unity Package
Apache License 2.0
245 stars 38 forks source link

Support runtime draco encoding #48

Closed camnewnham closed 2 years ago

camnewnham commented 2 years ago

Had a hard time trying to figure out why I couldn't access the DracoEncoder from other scripts. This minor change makes the Draco encoder accessible at in Assembly-C# (runtime).

According to the docs this shouldn't affect the build?

When you disable the Auto Reference option, Unity does not automatically reference the assembly during compilation. This has no effect on whether Unity includes it in the build.

atteneder commented 2 years ago

Hi @camnewnham,

First off: On top of the described change, the PR contains tons of other changes. I cannot merge it in this condition.

Nevermind, I didn't read the headline and thought it's just about the asmdef problem. Still I'd add the binaries myself for security reasons.

What's wrong with explicitly referencing the draco assembly? Sure, it forces you to use an asmdef yourself, but that's good practice anyways.

Granted, the docs could've mention this.

camnewnham commented 2 years ago

First off: On top of the described change, the PR contains tons of other changes. I cannot merge it in this condition.

Apologies, I mistakenly used my main branch for this PR. You can disregard the changes here which aggregate other open PRs

The only change for DracoUnity was to set the assembly to auto referenced. The bigger change was to add the appropriate assemblies from draco (see https://github.com/atteneder/draco/pull/8 for updated binary build actions) so that behaviour is consistent across platforms.

What's wrong with explicitly referencing the draco assembly? Sure, it forces you to use an asmdef yourself, but that's good practice anyways.

Granted, the docs could've mention this.

That sounds reasonable too; and yes having it in the docs would help! It took me a while to figure out why the namespace was missing - I expected export to be treated the same way as import.

atteneder commented 2 years ago

Apologies, I mistakenly used my main branch for this PR. You can disregard the changes here which aggregate other open PRs

OK, got it.

I expected export to be treated the same way as import

Oh shit, you're right. It's is inconsistent there :/

Anyways, I'll prefer to keep it as is right now. I cannot un-auto-reference the main assembly as it would break existing projects.

Thanks