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

Textures support to mtl files #7176

Closed
wants to merge 1 commit into from

Conversation

diyaayay
Copy link
Contributor

@diyaayay diyaayay commented Aug 15, 2024

Resolves #6924

Thanks to @rohanjulka19 for his prior work.
This PR is work in progress.

Changes:

  • Error handling in loading texture image.
  • clearTextures() as a draft.

Description:
These are some of the things that I'm not too sure about and would love to get some suggestions:

  • Should the class design of p5.Geometry be decoupled?
    I am not too sure but I think there should be some separation between p5.Geometry or multiple objects and their properties such as material properties, diffuse colors, textures, etc.
  • The clearTextures() might not be what we desire but it's just the idea of having it here. (ToDo)

Seperation of Concerns in the p5.Geometry class:

Class: p5.Geometry
Focuses solely on the geometric data of the 3D object.

  • Properties:
    • vertices
    • normals
    • faces
    • texCoords, etc.

and methods for the geometry data.

Class: p5.Material
Manages material properties, colors, texture, mtl properties, etc.
Methods like: applyMaterial(fill()), clearColors(), clearTextures(), etc.

A Grouping Structure:
Class: p5.Group : Combines multiple p5.Geometry, p5.Material and transform (global transformation on the entire group) and methods.

PR Checklist

@diyaayay diyaayay marked this pull request as draft August 15, 2024 11:29
@diyaayay diyaayay marked this pull request as ready for review August 27, 2024 16:44
@diyaayay
Copy link
Contributor Author

@davepagurek Should we decouple p5.Geometry class?
I've added a clearTextures(). Should there be fillTextures() like p5.js has a fill()?

@davepagurek
Copy link
Contributor

sorry for the delay on this! Overall I like your separation of concerns and think that direction makes sense. The group-like class would encompass multiple geometry objects so you can draw them all at once, and import them all at once, which is the bit most relevant to the original issue. That would mean that rather than importing multi-texture models as a single p5.Geometry, it would put the bits that use different textures in different geometry objects, and then return a single group containing all of them.

If we were to go that route, we'd also need to consider how buildGeometry would work with varied textures. Currently, buildGeometry will always return one p5.Geometry and does not record things like texture. I think that maybe means we'd want a separate thing that records and creates multiple p5.Geometry as needed? Would it be confusing having a separate thing for that? It might help writing some pretend code that we'd have the user write so we can think about the implications.

@diyaayay
Copy link
Contributor Author

sorry for the delay on this! Overall I like your separation of concerns and think that direction makes sense. The group-like class would encompass multiple geometry objects so you can draw them all at once, and import them all at once, which is the bit most relevant to the original issue. That would mean that rather than importing multi-texture models as a single p5.Geometry, it would put the bits that use different textures in different geometry objects, and then return a single group containing all of them.

If we were to go that route, we'd also need to consider how buildGeometry would work with varied textures. Currently, buildGeometry will always return one p5.Geometry and does not record things like texture. I think that maybe means we'd want a separate thing that records and creates multiple p5.Geometry as needed? Would it be confusing having a separate thing for that? It might help writing some pretend code that we'd have the user write so we can think about the implications.

I'm not sure if changing buildGeometry to return multiple p5.Geometry objects is the right approach at the moment, as it would affect a lot of things. So, I prefer the idea of having a separate thing that records and creates multiple p5.Geometry objects as needed. Later, we can incorporate that approach if desired. I'll look into some hypothetical user code to analyze how this should be implemented.

@diyaayay diyaayay closed this Nov 13, 2024
@diyaayay diyaayay deleted the textures-to-mtl branch November 13, 2024 18:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for .mtl Files with Textures
2 participants