google / draco

Draco is a library for compressing and decompressing 3D geometric meshes and point clouds. It is intended to improve the storage and transmission of 3D graphics.
https://google.github.io/draco/
Apache License 2.0
6.52k stars 966 forks source link

Handling of invalid values in encoding #358

Closed BerndGe closed 6 years ago

BerndGe commented 6 years ago

Hi,

I am using C++ code as below for adding a custom attribute to a Draco file. To get started, I added the code below to PlyDecoder::DecodeVertexData (after the handling of normals) in order to be able to just add the new attribute to the PLY file and re-use the existing code for PLY decoding.

The debug output in this code prints the correct values. However, the results which are provided by DRACOLoader (for three.js) are not what I expect.

  1. My input contains FLT_MAX (3.4028234663852886e+38) values for deviation which signalises an invalid value. Then the result values are always either the minimum of the input values or FLT_MAX
  2. If I replace all 3.4028234663852886e+38 occurences in the PLY file by "nan", the encoder crashes in ComputeShannonEntropy because of an out-of-bounds access to symbol_frequencies

Is there anything wrong with the code below? Am I supposed to mark invalid values differently? Any other ideas?

Background: The existing implementation of PLY decoding ignores everything except for some standard attributes.

My example file contains a header

  property float x
  property float y
  property float z
  property float deviation

and then for each vertex e. g.

  -25 -100.00000000582078 17.593749999672582 0.09277958422899246
  -25 -100.00000000582078 18.06249999978172 0.0835077241063118

C++ code:


  const PlyProperty *const deviation_prop = vertex_element->GetPropertyByName("deviation");
  if (deviation_prop != nullptr) {
    // For now, all normal properties must be set and of type float32
    if (deviation_prop->data_type() == DT_FLOAT32) {
      PlyPropertyReader<float> deviation_reader(deviation_prop);
      GeometryAttribute va;
      va.Init(GeometryAttribute::GENERIC, nullptr, 1, DT_FLOAT32, false,
              sizeof(float), 0);
      const int att_id = out_point_cloud_->AddAttribute(va, true, num_vertices);
      for (PointIndex::ValueType i = 0; i < num_vertices; ++i) {
        float val = deviation_reader.ReadValue(i);
        printf("deviation value %f\n", val);
        out_point_cloud_->attribute(att_id)->SetAttributeValue(
            AttributeValueIndex(i), &val);
      }
      std::unique_ptr<AttributeMetadata> deviation_metadata =
        std::unique_ptr<AttributeMetadata>(new AttributeMetadata());
      deviation_metadata->AddEntryString("name", "deviation");

      out_point_cloud_->AddAttributeMetadata(att_id, std::move(deviation_metadata));
    }
  }```
ondys commented 6 years ago

Thanks for the crash report. Ideally Draco should not crash even for "invalid" input so we will fix that.

That being said for your case, the crash most likely happens because you are quantizing the generic attribute. If the attribute is as floating point (quantization disabled) then I believe it would work correctly, but your compression ratio may suffer (Draco currently doesn't have a good support for compressing floating point attributes without quantization).

You could probably achieve what you want by replacing your "invalid" entries by some reasonable value (e.g. 0) and adding a new uint8 attribute that would mark the invalid vertices (i.e. store 1 for invalid vertex and 0 otherwise) .

BerndGe commented 6 years ago

Thanks. Indeed that seems to work, but the compression ratio is not good.