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

New version impedes PCX importer to work: v0.4.0: URP/HDRP support, easier PLY workflow, smaller chunk info Latest #15

Closed
carlosnavarro-cn opened this issue Oct 4, 2023 · 10 comments

Comments

@carlosnavarro-cn
Copy link

It's great to have this running in URP/HDRP but the new method of version 4 for creating the assets, through a ply importer, impedes other useful packages, as PCX for importing pointclouds, from working simultaneously in the scene, which wasn't the case until version 3. Would it be possible to incorporate the GaussianSplatURPFeature or GaussianSplatHDRPPass in version 3 that didn't have this conflict? Thank you!

As of now, this error occurs, when having both packages installed:

i get this error in unity: Multiple scripted importers are targeting the extension 'ply' and have all been rejected: Pcx.PlyImporter (assembly: C:\Unity_New\UnityGaussianSplatting-main\Library\ScriptAssemblies\Pcx.Editor.dll), GaussianPlyImporter (assembly: C:\Unity_New\UnityGaussianSplatting-main\Library\ScriptAssemblies\GaussianSplatting.dll)
UnityEditor.AssetImporters.ScriptedImporter:RegisterScriptedImporters ()

@aras-p
Copy link
Owner

aras-p commented Oct 4, 2023

Ah! I guess I could make it so that people have to rename their gaussian splat point clouds to some other extension, e.g. .gply. Would that work?

@andybak
Copy link

andybak commented Oct 4, 2023

Not the original poster but I think it would. However I think this points to a flaw somewhere. On one level - the Splatting community should probably settle on their own extension. These are regular .ply files only in the loosest sense so a new extension makes a lot of sense and would ideally be adopted widely.

But another aspect is that Unity's scripted importers are really annoying! This isn't the first time the tight coupling between importer and file extension has caused problems.

@aras-p
Copy link
Owner

aras-p commented Oct 4, 2023

@andybak looks like Unity scripted importers do have some functionality where multiple importers could co-exist. I'll investigate https://docs.unity3d.com/ScriptReference/AssetDatabase.SetImporterOverride.html and friends and will report back. Thanks for notifying me, I was not aware that other packages also want to import PLY files!

@carlosnavarro-cn
Copy link
Author

carlosnavarro-cn commented Oct 4, 2023

Ah! I guess I could make it so that people have to rename their gaussian splat point clouds to some other extension, e.g. .gply. Would that work?

Thanks, I imagine that might work. PCX is very established for PLY, it has a lot of functionalities and works with VFX. I’m eager to produce visuals using both packages. I was able to add the GaussianSplatURPFeature to an asset renderer on version 3, but it only worked by replacing the GaussianSplatRenderer and deleting the GaussianAssetCreator, but my previous created assets still don’t visualize. The GaussianSplatRenderer panel even shows a message that say the asset was created in a previous version. Thank you for considering this issue!

@aras-p
Copy link
Owner

aras-p commented Oct 4, 2023

I was hoping that using ScriptedImporterAttribute.overrideFileExtensions would work nicely, as in I'd register no "primary" extension, only the ply as an override extension. And then indeed it works nicely, if both GaussianSplat and PCX are in the project - PCX is used by default, but you can override the asset to be imported as GaussianSplat instead.

But! Then it does not work at all if PCX is not in the project; the asset just is not imported at all. Oooof!

I guess I'll get back to not using a custom importer afterall, i.e. back to the "create asset" window thingy.

@carlosnavarro-cn
Copy link
Author

Thank you for trying that! Yes, the create asset window, prior to the URP/HDRP update, worked great with PCX.

aras-p added a commit that referenced this issue Oct 4, 2023
Unity does not provide a good way for multiple importers for the same
extention to co-exist. And turns out pretty popular "PCX" point cloud
package already exists that has claimed .ply.
@aras-p
Copy link
Owner

aras-p commented Oct 4, 2023

Should be fixed now in v0.5.0

@aras-p aras-p closed this as completed Oct 4, 2023
@carlosnavarro-cn
Copy link
Author

Should be fixed now in v0.5.0

Thank you so much for implementing this, @aras-p . It works!

Sometimes I've had some memory leakage alerts when selecting ply files on the project window, but I've been able to ignore them. Runtime works normally.

[00:07:17] [Worker1] Internal: There are remaining Allocations on the JobTempAlloc. This is a leak, and will impact performance
[00:07:17] [Worker1] To Debug, run app with -diag-job-temp-memory-leak-validation cmd line argument. This will output the callstacks of the leaked allocations.
[00:07:26] [Worker4] Internal: There are remaining Allocations on the JobTempAlloc. This is a leak, and will impact performance
[00:07:26] [Worker4] To Debug, run app with -diag-job-temp-memory-leak-validation cmd line argument. This will output the callstacks of the leaked allocations.
[Worker4] To Debug, run app with -diag-job-temp-memory-leak-validation cmd line argument. This will output the callstacks of the leaked allocations.
A [Worker4] To Debug, run app with -diag-job-temp-memory-leak-validation cmd line argument. This will output the callstacks of the leaked allocations.

@hybridherbst
Copy link
Contributor

One thing I did so the various glTF importers could happily co-exist was to make a scripting define to specify if the „fileExtension“ should be used (default) or if „overrideFileExtension“ is used.

This allows for both, the nice import workflow and coexisting importers. People that have both pcx and splats have to set the scripting define manually (can also be automatic with a version define in an AsmDef).

@aras-p
Copy link
Owner

aras-p commented Oct 8, 2023

@hybridherbst yeah that might work, but also a step that someone has to actually do :/ Right now I'll probably keep on not using a scripted importer, given that it has other issues (namely: there's no way to efficiently store the imported "bunch of bytes" without making either inspector slow, or domain reload slow -- waiting for Unity to add TextAsset constructor that takes byte array as input).

dylanebert pushed a commit to dylanebert/UnityGaussianSplatting that referenced this issue Oct 13, 2023
Unity does not provide a good way for multiple importers for the same
extention to co-exist. And turns out pretty popular "PCX" point cloud
package already exists that has claimed .ply.
HIHHIYAYAYOO pushed a commit to HIHHIYAYAYOO/UnityGaussianSplatting that referenced this issue Nov 30, 2023
Unity does not provide a good way for multiple importers for the same
extention to co-exist. And turns out pretty popular "PCX" point cloud
package already exists that has claimed .ply.
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

No branches or pull requests

4 participants