ieskudero / three-dxf-viewer

DXF viewer using ThreeJS
https://www.npmjs.com/package/three-dxf-viewer
MIT License
61 stars 23 forks source link

non-BLOCK, INSERT or DIMENSION entities in blocks position fixed #10

Closed aceway closed 3 months ago

aceway commented 3 months ago

The DXF viewer always shows some incorrectly positioned objects in the scene when there are non-BLOCK, INSERT or DIMENSION entities in blocks in a DXF file. Their position may be far from where they need to be.

The blockEntity.js merges them but does not apply transformations to them.

ieskudero commented 3 months ago

Can you provide a test dxf file to verify this? It will help a lot

aceway commented 3 months ago

Can you provide a test dxf file to verify this? It will help a lot

The file is here: axis2013.dxf.tar.gz

You could not see those exception objects within the online demo, because they are too small and too far away. Maybe you should create the scene box to location them on the edges and them zoom in, I found them in this way.

ieskudero commented 3 months ago

Thanks for the file.

After running some tests, I can't see what does this PR changes. I would need a more detailed explanation on what changes on the file (looks the same with and without the changes). Or a more simpler file containing only the affected block and entities.

Also, I don't understand why it should be on the Merger class. Shouldn't it be on the BlockEntity class?

aceway commented 3 months ago

Thanks for the file.

After running some tests, I can't see what does this PR changes. I would need a more detailed explanation on what changes on the file (looks the same with and without the changes). Or a more simpler file containing only the affected block and entities.

Also, I don't understand why it should be on the Merger class. Shouldn't it be on the BlockEntity class?

Sorry, I can not give a more simple file. I found the bug in some big dxf files: there are non-BLOCK, INSERT or DIMENSION entities in blocks(non-block and block together inserted into another block).

I fixed it in Merger class, because the two type objects (merged lines and meshes) are generated in the Merger class and added into the scene(group) directly, so could not transform them in the BlockEntity class(after merged). Of cause if you wrapped the two generated line and mesh into a new group and not add them into the scene directly but return the new group, then you can apply transformations in BlockEntity class.

The attatched images are another huge dxf file encounter this bug: 123

aceway commented 3 months ago

@ieskudero Hi there,

How about this? If you don't want it, I will keep it and fix some other problems (memory waste and crashes in the mobile environment) in my forked repo locally.

ieskudero commented 3 months ago

Hi @aceway,

I understand there are many bugs that need to be fixed, and this may be one of them, However, I want to clarify some points:

The way I see it, this pull request have some problems to be merged:

Having said that, how about this: I believe that your contribution fixes the bug, so I am going to create a new class that only the BlockEntity will use, based on the Merger class, containing your fix. This will allow to fix the bug and keep the implications to a minimun.

I know this come late, and I am sorry about it. Hope this helps and thank you for your effort.

aceway commented 3 months ago

Hi @aceway,

I understand there are many bugs that need to be fixed, and this may be one of them, However, I want to clarify some points:

  • I usually check this repo once in a while, unfortunately I don't have much time to mantain it.
  • I try to fix small problems or problems that I can reproduce, or see.

The way I see it, this pull request have some problems to be merged:

  • There is no file that shows the bug, or I am not able to reproduce or see the error. Bug example files should only contain the minimun amount of entities in order to find and see the bug. I know how hard it is to get them, but I think is necessary.
  • It changes a general class that can be used in many other places, so I can't measure the implications of changing it.

Having said that, how about this: I believe that your contribution fixes the bug, so I am going to create a new class that only the BlockEntity will use, based on the Merger class, containing your fix. This will allow to fix the bug and keep the implications to a minimun.

I know this come late, and I am sorry about it. Hope this helps and thank you for your effort.

I cannot provide a simple dxf file for you to reproduce it, you are right, maybe my fixed code just only solved the problums with my dxf files, so I give up thre pull request. There is no need to create an extra class to accepted my pull request.