Support for .mtl Files with Textures #7072
Conversation
|
🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page! |
rohanjulka19
changed the title
rohanjulka19
changed the title
rohanjulka19
changed the title
|
Hi, sorry for the delay in reviewing this! I'm leaving a comment here to bump this to the top of my pile, I'll get back to you shortly! |
There was a problem hiding this comment.
Thanks for taking this on! This definitely starts to expose some of the limits of the current design for p5.Geometry, so unfortunately instead of just making this new feature it kind of means also discussing the class design and how we expect it to grow in the future before deciding.
I've left a few comments about ways to try to maybe keep some consistency, but we've also floated the idea in the past of having a separate data structure that acts as a group of multiple p5.Geometry objects combined with other p5 settings (material properties, textures, etc) and this maybe would be a good time to start doing that split. Let me know what you think!
| } | ||
| model.hasTextures = true; | ||
| model.textures[currentMaterial].diffuseTexture = | ||
| loadImage(materials[currentMaterial].diffuseTexturePath); |
There was a problem hiding this comment.
What do we expect to happen here if this image file hasn't been uploaded along with the sketch? Maybe we can add some error handling here?
| @@ -1125,7 +1143,19 @@ p5.prototype.model = function(model) { | |||
| this._renderer.createBuffers(model.gid, model); | |||
| } | |||
|
|
|||
| this._renderer.drawBuffers(model.gid); | |||
| // If model has textures for each texture update index buffer and invoke draw call. | |||
| if(model.hasTextures) { | |||
There was a problem hiding this comment.
I wonder if we can make this a little more consistent with fill/stroke? So previously, p5.Geoemtry was kind of just shape info, not including color or textures, and you would be able to call fill() or stroke() before model(yourModel) before drawing it to change those things. Now, buildGeometry captures some of those vertex and stroke colors, so they no wouldn't get reset, they're baked in. If you want to turn your shape back into something without that info baked in, we have a geom.clearColors() method you can call. For this scenario that maybe means:
- Having a method to clear the textures out of geometry (either along with or independently from the colors)
- Capturing the textures, if any are applied, in
buildGeometry
| this._renderer.drawBuffers(model.gid); | ||
| for (let material of Object.keys(model.textures)) { | ||
| const texture = model.textures[material]; | ||
| this._renderer.updateIndexBuffer(model.gid, texture.faces); |
There was a problem hiding this comment.
Is there a way for us to do this by having multiple index buffers that we swap between? In the past we've found that on some computers, the cpu-to-gpu data transfer is by far the biggest bottleneck, so we've tried to minimize the data sent back and forth. Currently, p5.Geometry is the way to cache this reused data to avoid any transfer after the first time you draw it, so ideally we'd keep that property.
| /** | ||
| * When using textures each material contains all the face indices for that texture. | ||
| * this function collects all the faces mapped to different textures in a single array | ||
| * @method collectFaces |
There was a problem hiding this comment.
Is this meant to be used internally only? If so we can add the @private tag so it doesn't show up in the reference for users.
|
Hi @rohanjulka19 , |
|
Hi @diyaayay, |
Resolves #6924
Changes:
This change allows to render diffuse textures specified in the .mtl files while rendering the model. To implement this, a JS object has been added to the Geometry class which maps each material name to a texture (p5.Img object) and the faces to which the texture has to be applied. While rendering the model first it is checked if the model has textures, if it does then for each texture the index buffer is updated with face indices for that texture and rendered. This runs in a loop until all the textures have been rendered.
I have moved out from the logic for creating and updating indexBuffer from the createBuffers function to updateIndexBuffer function because I only wanted to update the index buffer again and again without deleting and initializing other buffers.
Screenshots of the change:
The mario obj file in the sample sketch specified in the issue have negative texture coordinates. Due to that it does not render correctly. Only when V coordinate is not inversed and texture wrapping mode is changed from clamp to repeat the model renders correctly. Now I am not sure exactly why this is working. For wrapping modes I understand why repeat mode should work but I am not sure how, not inversing the V coordinate is fixing the problem.
PR Checklist
npm run lintpassesPS: I am not sure if this was the exepcted implementation. If this looks fine I will add unit tests for this.