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

DXF: Support negative index in VERTEX #5118

Merged
merged 1 commit into from Sep 1, 2023

Conversation

kenichiice
Copy link
Contributor

In DXF files, negative value is allowed for the "Polyface mesh vertex index" of the VERTEX entity.

https://help.autodesk.com/view/ACD/2024/ENU/?guid=GUID-0741E831-599E-4CBF-91E1-8ADBCFD6556D

If the index is negative, the edge that begins with that vertex is invisible.

However, the current implementation of DXFImporter::ParsePolyLineVertex() converts negative index to 0 and cannot handle them correctly.

This pull request ensures that negative index is handled correctly by reading their absolute value.

@turol
Copy link
Member

turol commented Jun 5, 2023

If the edge is supposed to be invisible shouldn't it be marked somehow or skipped?

@kenichiice
Copy link
Contributor Author

The current patch only prevents the loss of faces. Still, I believe it improves the function.

As you mentioned, the invisible information should be retained if possible. Unfortunately, I haven't been able to implement this as I'm unsure how to represent invisible edges in assimp.

@kenichiice
Copy link
Contributor Author

I think this pull request will fix the next issue:

@kenichiice
Copy link
Contributor Author

I have created an example of a DXF file that cannot be read correctly without applying this PR.

This file defines a square mesh consisting of two triangles. Negative indices are specified on lines 476 and 490.

When testing the code provided below, the behavior is as follows.

Using assimp before applying the PR:

num meshes = 1
mesh 0
  vertices:
    1, 1, -1
    1, 1, 1
    1, -1, 1
    1, -1, -1
  faces:
    0, 1
    2, 3

It only has 2 face vertices and does not read the triangle.

Using assimp after applying the PR:

num meshes = 1
mesh 0
  vertices:
    1, 1, -1
    1, 1, 1
    1, -1, 1
    1, 1, -1
    1, -1, 1
    1, -1, -1
  faces:
    0, 1, 2
    3, 4, 5

This one reads triangles.

The code used is as follows:

#include <iostream>

#include <assimp/Importer.hpp>
#include <assimp/scene.h>

int main(int argc, char* argv[])
{
  if (argc != 2) {
    std::cerr << "usage: " << argv[0] << " FILE" << std::endl;
    return 2;
  }

  Assimp::Importer importer;
  const auto* scene = importer.ReadFile(argv[1], 0);

  if (!scene) {
    std::cerr << "failed to read file" << std::endl;
    return 1;
  }

  std::cout << "num meshes = " << scene->mNumMeshes << std::endl;
  for (unsigned i = 0; i < scene->mNumMeshes; ++i) {
    const auto* m = scene->mMeshes[i];

    std::cout << "mesh " << i << std::endl;

    std::cout << "  vertices:" << std::endl;
    for (unsigned j = 0; j < m->mNumVertices; ++j) {
      const auto& v = m->mVertices[j];
      std::cout << "    " << v.x << ", " << v.y << ", " << v.z << std::endl;
    }

    std::cout << "  faces:" << std::endl;
    for (unsigned j = 0; j < m->mNumFaces; ++j) {
      const auto& f = m->mFaces[j];
      std::cout << "    ";
      for (unsigned k = 0; k < f.mNumIndices; ++k) {
        std::cout << f.mIndices[k];
        if (k + 1 < f.mNumIndices) {
          std::cout << ", ";
        } else {
          std::cout << std::endl;
        }
      }
    }
  }
}

@kenichiice
Copy link
Contributor Author

The original patch conflicts with the changes made in PR #5146.
So I reworked the patch to match the latest code.

@turol
Copy link
Member

turol commented Aug 31, 2023

Prefer rebase over merge, makes for a cleaner history.

@kenichiice
Copy link
Contributor Author

I cleaned up the history by rebasing.

@kimkulling kimkulling merged commit 0ff0bd7 into assimp:master Sep 1, 2023
11 checks passed
@kimkulling
Copy link
Member

Merged, thanks a lot for your contribution.

@kenichiice kenichiice deleted the dxf/negative-index branch September 1, 2023 15:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants