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

JSON export bugfixes #121

Merged

Conversation

Floppy
Copy link
Contributor

@Floppy Floppy commented Feb 25, 2024

I was trying to debug my 3MF exporter using to_json, and found a few bugs that tripped it up. These changes fix those, so that running to_json on a loaded male02.obj file now works. No extra tests added, just a few quick tactical fixes that seem fairly obvious.

Incidentally, to_json in other contexts would normally produce a string, whereas this creates a hash which then needs JSON.generate() calling on it. Maybe something to reconsider down the line, what the "correct" output from to_json should be.

@Floppy
Copy link
Contributor Author

Floppy commented Feb 26, 2024

The test failure here is because there is an extra level of object in the children array now. Rather than just containing objects, it contains complete to_json output from the child objects, including a metadata section. I'll update the code to pass, but I'm wondering, what should this be outputting? Should the metadata be in there? Or should it just be the contents of the "object" part of the child?

@Floppy
Copy link
Contributor Author

Floppy commented Feb 28, 2024

Trying to phrase my question better. Which is the correct output?

{
  :metadata=>{
    :version=>4.3, 
    :type=>"Object", 
    :generator=>"ObjectExporter"
  }, 
  :object=>{
    :uuid=>"c52c90fd-8f3c-43ec-967f-4714b6a4a482", 
    :type=>"Object3D", 
    :matrix=>[1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0], 
    :children=>[
      {
        :uuid=>"cedd4b1e-e86d-4eb2-8591-314c5cec2e2a", 
        :type=>"Object3D", 
        :matrix=>[1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0]
      }
    ]
  }
}

or

{
  :metadata=>{
    :version=>4.3, 
    :type=>"Object", 
    :generator=>"ObjectExporter"
  }, 
  :object=>{
    :uuid=>"c52c90fd-8f3c-43ec-967f-4714b6a4a482", 
    :type=>"Object3D", 
    :matrix=>[1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0],
    :children=>[
      {
        :metadata=>{
          :version=>4.3, 
          :type=>"Object", 
          :generator=>"ObjectExporter"
        }, 
        :object=>{
          :uuid=>"cedd4b1e-e86d-4eb2-8591-314c5cec2e2a", 
          :type=>"Object3D", 
          :matrix=>[1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0, 0.0, 0.0, 0.0, 0.0, 1.0]
        }
      }
    ]
  }
}

@danini-the-panini danini-the-panini added this to the 0.4.1 milestone Mar 8, 2024
@danini-the-panini
Copy link
Owner

The first one, we don't need the metadata on every child object

@Floppy
Copy link
Contributor Author

Floppy commented Mar 8, 2024

Great, thanks, I'll update the code :)

json = object.to_json
assert_equal('Mesh', json['object']['children'][0]['type'])
# Check geometry node and uuid references
assert_equal('Geometry', json['geometries']['children'][0]['type'])
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@danini-the-panini To make sure this is right, I'm adding specific tests for mesh and line children, and I'm not sure if the behaviour I'm going for is correct.

Do ALL geometries and materials live in a single array at the root of the JSON, no matter the object nesting? Then the objects themselves reference them by UUIDs further down in the tree? So, the geometry and material for an object are NOT contained anywhere under the object/children array?

That's what it looks like it should be, from things like the Line and Mesh test_to_json tests, but I thought I'd check.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the objects exist in the children array where they're nested

Copy link
Contributor Author

@Floppy Floppy Mar 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, interesting, in that case there would need to be another level in there, different to the current states. I think these two examples show the choice. The first one is in line with the current nesting method, whereas the second needs another level in there but is in line with existing mesh to_json output being wholly contained within the children array.

Sorry if this is confusing 😁

{
  geometries: [
    {
      uuid: "abc123",
      type: 'Geometry',
      data: {
        vertices: [],
        normals: [],
        faces: []
      }
    },
    {
      uuid: "bdc234",
      type: 'Geometry',
      data: {
        vertices: [],
        normals: [],
        faces: []
      }
    }
  ],
  materials: [
    {
      uuid: "def456",
      type: 'MeshBasicMaterial'
    },
    {
      uuid: "efg567",
      type: 'LineBasicMaterial'
    }
  ],
  object: {
    uuid: "c52c90fd-8f3c-43ec-967f-4714b6a4a482",
    type: "Object3D",
    children: [
      {
        uuid: "cedd4b1e-e86d-4eb2-8591-314c5cec2e2a",
        geometry: "abc123",
        material: "def456",
        type: "Mesh"
      },
      {
        uuid: "cedd4b1e-e86d-4eb2-8591-314c5cec2e2a",
        geometry: "bcd234",
        material: "efg567",
        type: "Line"
      }
    ]
  }
}

vs

{
  object: {
    uuid: "c52c90fd-8f3c-43ec-967f-4714b6a4a482",
    type: "Object3D",
    children: [
      {
        geometries: [
          {
            uuid: "abc123",
            type: 'Geometry',
            data: {
              vertices: [],
              normals: [],
              faces: []
            }
          }
        ],
        materials: [
          {
            uuid: "def456",
            type: 'MeshBasicMaterial'
          }
        ],
        object: {
          geometry: "abc123",
          uuid: "cedd4b1e-e86d-4eb2-8591-314c5cec2e2a",
          material: "def456",
          type: "Mesh"
        }
      },
      {
        geometries: [
          {
            uuid: "bdc234",
            type: 'Geometry',
            data: {
              vertices: [],
              normals: [],
              faces: []
            }
          }
        ],
        materials: [
          {
            uuid: "efg567",
            type: 'LineBasicMaterial'
          }
        ],
        object: {
          uuid: "cedd4b1e-e86d-4eb2-8591-314c5cec2e2a",
          geometry: "bcd234",
          material: "efg567",
          type: "Line"
        }
      }
    ]
  }
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh i see, i misunderstood.
yeah there should just be the top level materials and geometries, then the children objects just reference them by uuid.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks! It was a difficult question to phrase, no worries :)

@Floppy
Copy link
Contributor Author

Floppy commented Nov 13, 2024

I'll revisit this once #125 is merged, to avoid conflicts.

@Floppy
Copy link
Contributor Author

Floppy commented Nov 14, 2024

OK, this is good to go now. 🎉

Could merge before or after #125, I'll whichever goes in second.

@danini-the-panini danini-the-panini merged commit dd0467c into danini-the-panini:main Nov 17, 2024
21 checks passed
danini-the-panini pushed a commit that referenced this pull request Nov 17, 2024
@Floppy Floppy deleted the misc-to-json-fixes branch November 17, 2024 21:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants