atteneder / DracoUnity

Draco 3D Data Compression Unity Package
Apache License 2.0
247 stars 39 forks source link

POINTS import: Invalid vertex attribute format+dimension #45

Closed camnewnham closed 12 months ago

camnewnham commented 2 years ago

This error occurs when using some sample files from the draco repository, such as https://github.com/google/draco/blob/master/testdata/pc_kd_color.drc

Full error text:

ArgumentException: Invalid vertex attribute format+dimension value (UNorm8 x 3, data size must be multiple of 4)
UnityEngine.Mesh+MeshData.SetVertexBufferParams (System.Int32 vertexCount, UnityEngine.Rendering.VertexAttributeDescriptor[] attributes) (at <07c89f7520694139991332d3cf930d48>:0)
Draco.DracoNative.CreateMesh (System.Boolean& calculateNormals, System.Boolean requireNormals, System.Boolean requireTangents, System.Int32 weightsAttributeId, System.Int32 jointsAttributeId, System.Boolean forceUnityLayout) (at Packages/com.atteneder.draco@4041c676d1/Runtime/Scripts/DracoNative.cs:540)

At a glance, it seems that Unity requires multiples of 4 for vertex attributes, but (presumably) these draco encoded attributes are 3xUInt8 (R,G,B instead of R,G,B,A).

What's the best way to go about converting this into a format Unity is friendly with?

atteneder commented 2 years ago

It's probably best to fill in alpha values of 255 before passing the vertex buffer to Unity. Question is at what stage (in the C++ wrapper or in managed C#).

camnewnham commented 2 years ago

Here's a brief test of this in these PRs (for discussion purposes)

This works by: DracoUnity - determines whether padding should be applied, using a PaddedColorAttributeMap. Passes the padded number of components to native. Draco - has an additional parameter to the GetAttributeData method which specifies an override for the stride of num_components.

At the moment, I'm not infilling with 255 as this would require a performance hit in C# or a bit more mess in C++. Most unlit shaders don't support alpha...

This fixes: https://github.com/google/draco/blob/master/testdata/pc_kd_color.drc https://github.com/google/draco/blob/master/testdata/point_cloud_no_qp.drc


There are unrelated issues with these: https://github.com/google/draco/blob/master/testdata/cube_pc.drc https://github.com/google/draco/blob/master/testdata/pc_color.drc

These fail at (*decoder)->DecodePointCloudFromBuffer(*buffer), and I am not sure why...

camnewnham commented 2 years ago

Ah, looking at the 'unrelated issues' above, these are failing because DRACO_BACKWARDS_COMPATIBILITY_SUPPORTED is not enabled; so I don't see this as a particular issue, though it's worth noting as they won't be able to be included in the unit tests.

Context is point_cloud_decoder.cc

  // Check for version compatibility.
#ifdef DRACO_BACKWARDS_COMPATIBILITY_SUPPORTED
  ...
#else
  if (version_major_ != max_supported_major_version) {
    return Status(Status::UNKNOWN_VERSION, "Unsupported major version."); // Hits this line
  }
...
#endif
atteneder commented 2 years ago

@camnewnham Yeah, not supporting older Draco files was a deliberate design decision (to achieve a smaller binary). Devs can recompile the libs with this flag on, if they absolutely need to load older files.

AR-Vsx commented 2 years ago

@camnewnham When using your fork in Unity, my captured point cloud looks like this : Screenshot 2022-05-09 123228 I'm creating the point cloud in a C++ Application like this:

builder.Start(capturedPC->count());
int pos_att_id = builder.AddAttribute(draco::GeometryAttribute::POSITION, 3,draco::DT_FLOAT32);
int color_att_id = builder.AddAttribute(draco::GeometryAttribute::COLOR, 3, draco::DT_UINT8);

for (draco::PointIndex i(0); i < capturedPC->count(); i++) 
{            
    builder.SetAttributeValueForPoint(pos_att_id, i, &points[i.value()]);
    builder.SetAttributeValueForPoint(color_att_id, i, &points[i.value()].r);
}

std::unique_ptr<draco::PointCloud> pc = builder.Finalize(true);

draco::Encoder e;
draco::EncoderBuffer eBuff;

e.EncodePointCloudToBuffer(*pc, &eBuff);
draco::WriteBufferToFile(ebuff.data(), ebuff.size(), pcEncFilename)

The points struct looks like this:

struct cwipc_point {
    float x;        /**< coordinate */
    float y;        /**< coordinate */
    float z;        /**< coordinate */
    uint8_t r;      /**< color */
    uint8_t g;      /**< color */
    uint8_t b;      /**< color */
    uint8_t tile;   /**< tile number (can also be interpreted as mask of contributing cameras) */
};

I'm also exporting the captured pointcloud as a ply file, which I encode with draco_encoder.exe -point_cloud and when I decode this file in Unity it looks fine. And when I decode the created drc file with draco_decoder.exe it also looks fine. To render the color of the point cloud in Unity I'm using https://github.com/keijiro/Pcx

camnewnham commented 2 years ago

When using your fork in Unity

@AR-Vsx just to confirm, did you build the matching native Draco library? The native dlls are not included in this PR (for security only those built by the CI are included). https://github.com/atteneder/draco/pull/6

Please let me know if that resolves your issue

AR-Vsx commented 2 years ago

I have built dracodec_unity.dll from https://github.com/camnewnham/draco. Is that correct ? If it is, it didn't fix the issue.

atteneder commented 2 years ago

I have built dracodec_unity.dll from https://github.com/camnewnham/draco. Is that correct ?

Maybe. The PR references branch color-pad. Have you used this exact branch?

AR-Vsx commented 2 years ago

No, I used the main branch. I switched branches, built the dll and it works fine now, Thank you and @camnewnham thank you too.

atteneder commented 1 year ago

Undoing unjust GH auto-close

atteneder commented 12 months ago

If I'm not mistaken this was fixed with binaries 1.1.0 in DracoUnity 3.4.0 and 4.1.0 respectively.