.mtl files support for .obj files to render color in 3D custom geometry by diyaayay · Pull Request #6710 · processing/p5.js

diyaayay · 2024-01-07T21:13:31Z

Resolves #6670
This PR is a Work in Progress.
Review is requested.

Changes:

Screenshots of the change:
The output after loading materials before the model is loaded and assigning diffuse colors to vertexColors
The output contains parsedMaterials and model.vertexColors

PR Checklist

npm run lint passes
Inline documentation is included / updated
Unit tests are included / updated

diyaayay · 2024-01-07T21:19:46Z

@davepagurek The error above in the screenshots occur only when an obj file with an mtl file associated with it is loaded.
I think the self._decrementPreload() in this case is where I'm unable to catch the correct instance of this to use and assign to self as a lot of asynchronous calls were made implement loading of materials before model.

The sketch used is:

let octa;

function preload() {
   octa = loadModel('octa-color.obj');
}

function setup() {
  createCanvas(400, 400, WEBGL);
}

function draw() {
  background(200);
  rotateX(frameCount * 0.01);
  rotateY(frameCount * 0.01);
  model(octa);
}

Requesting a review so that I can proceed further and also guidance in solving the current error.

diyaayay · 2024-01-07T21:48:39Z

Sorry for the commit messages, I thought I had already committed some other changes earlier.

…y/p5.js into Mtl-support-to-obj-files

davepagurek

Great work so far! I've left a few comments, some about the error you were running into, and some about how we build up the model after that. Let me know what you think!

davepagurek · 2024-01-11T12:20:40Z

src/webgl/loading.js

+  return new Promise((resolve,reject)=>{
+    // console.log('parser is called');
+    let currentMaterial = null;
+    p5.prototype.loadStrings(


I think rather than calling this on p5.prototype, which will have no this, you'd want to call loadStrings on a p5 instance. That could mean calling await parseMtl(this, mtlPath); inside loadModel and then call loadStrings on the first argument, although it might be a nicer separation of concerns to separate loading the strings and the parsing, so it becomes:

this.loadStrings( mtlPath, (mtlLines) => { const materials = parseMtl(mtlLines); parseObj(model, lines, materials) }, () => { // handle failure } )

Another benefit of this is that you maybe don't need parseMtl to resolve or reject, since the loadStrings part handles the case where it can't fetch the file, so parseMtl can look more like parseObj.

davepagurek · 2024-01-11T12:23:19Z

src/webgl/loading.js

 * @return {p5.Geometry} the <a href="#/p5.Geometry">p5.Geometry</a> object
 */
+
+let materials = {}; //currently local


Do we use this btw? Looks like we always pass in materials as a parameter

Ah, yes I had to remove it , missed it though.

davepagurek · 2024-01-11T12:26:10Z

src/webgl/loading.js

-        if (normalize) {
-          model.normalize();
+      async lines => {
+        const mtlLine = lines.find(line => line.startsWith('mtllib '));


I think the obj spec supports multiple mtls, so we might need to filter to get all the lines that have mtllib and then parse all of them (maybe waiting for them all via Promise.all(...)?)

I thought there could be just one mtl file to an obj file. After searching it through, yes there can be more. filter should be used . Thanks.

davepagurek · 2024-01-11T12:26:43Z

src/webgl/loading.js

+      lines => {
+        for (let line = 0; line < lines.length; ++line){
+          const tokens = lines[line].trim().split(/\s+/);
+          if(tokens[0] === 'newmtl') {


Nice work on this parser, looks great!

davepagurek · 2024-01-11T12:29:35Z

src/webgl/loading.js

+        currentMaterial = tokens[1];
+        if (currentMaterial && materials[currentMaterial]) {
+          const diffuseColor = materials[currentMaterial].diffuseColor;
+          model.vertexColors.push([


I think we want to add to the diffuse colors for each vertex, where currently this is adding to the array once for each usemtl line. So maybe that means just setting the currentMaterial in this if branch, and in the branch below where we add to loadedVerts, add to vertexColors based on the material.

Oh yes, the logic in my head was a little haywire and that's why I added to the array to vertexColors for each usemtl . Got your point, thanks for explaining the changes to be made also.

davepagurek · 2024-01-11T12:33:25Z

src/webgl/loading.js

+            diffuseColor[2]
+          ]);
+        } else {
+          model.vertexColors.push([1, 1, 1]);


Just thinking this through: when a model has vertex colors, then fill() has no effect on the model, unless you call .clearColors() on it. Currently, all imported models have no colors, so they take on whatever fill is used. We want to keep that behaviour for objs that have no materials, so maybe that means if no materials are present in the obj, rather than pushing to vertexColors, we just leave it empty.

This would make for a good unit test btw: testing that if you import an obj that has materials, it uses the material colors when you draw it with model(), and when you import an obj that has no materials, it draws using whatever the fill color is.

davepagurek · 2024-01-11T12:35:10Z

test/unit/assets/octa-color.obj

@@ -0,0 +1,23 @@
+mtllib octa-color.mtl


The plant obj is 9mb, which might slow down tests. Maybe for the missing material test, we can make a duplicate of this colored obj, but change the path from octa-color.mtl to something that doesn't exist?

yes, that's what I had thought of too, made the same changes already for plant.obj and will push them asap.

davepagurek · 2024-01-14T15:44:31Z

Btw just a heads up, looks like this test is failing:

davepagurek

Btw, we also recently merged in support for visual tests. That might be a good fit here to check that vertex colors are visible on canvas when drawing.

davepagurek · 2024-01-14T15:47:14Z

src/webgl/loading.js

+              mtlPath=tokens[1];
+            }
+            // Check if the MTL file has already been loaded
+            const existingMtl = mtlFiles.find(mtl => mtl.path === mtlPath);


I think this is a bit of a race condition, since we only push to the array after awaiting an asynchronous function. Maybe you can synchronously push an object that just includes the path, and then after awaiting, add the materials to it? something like:

const file = { path: mtlPath }; mtlFiles.push(file); const parsedMaterials = await parseMtl(self, mtlPath) file.materials = parsedMaterials;

davepagurek · 2024-01-14T15:49:57Z

src/webgl/loading.js

-            //Texture path
-            materials[currentMaterial].texturePath = tokens[1];
-          }
+async function parseMtl(self,mtlPath){ //accepts mtlPath to load file


Maybe we can call self p5 instead so it's clearer what its type should be?

davepagurek · 2024-01-14T15:53:46Z

src/webgl/loading.js

+async function parseMtl(self,mtlPath){ //accepts mtlPath to load file
+  let materials= {};
+  let currentMaterial = null;
+  self.loadStrings(


It looks like this function completes without waiting for the result of loadStrings now that it no longer returns a promise. So I think that means it will immediately return {}, and then asynchronously, properties will get added to it. I'm guessing it's important to have the complete materials object before continuing to parse .obj files later, so maybe it would make sense to add back in something like you had before?

const materials = {}; const doneLoading = new Promise((resolve, reject) => { self.loadStrings( mtlPath, lines => { // do parsing and add to materials here resolve(); }, reject ) }); await doneLoading; return materials;

@davepagurek I changed my multiple mtl file support functionality to this:

else if (fileType.match(/\.obj$/i)) { this.loadStrings( path, async lines => { const parsedMaterialPromises=[]; try{ for (let i = 0; i < lines.length; i++) { const mtllibMatch = lines[i].match(/^mtllib (.+)/); if (mtllibMatch) { let mtlPath=''; const mtlFilename = mtllibMatch[1]; const objPathParts = path.split('/'); if(objPathParts.length > 1){ objPathParts.pop(); const objFolderPath = objPathParts.join('/'); mtlPath = objFolderPath + '/' + mtlFilename; }else{ mtlPath = mtlFilename; } const parsedMaterialsIndividual = await parseMtl(self, mtlPath); parsedMaterialPromises.push(parsedMaterialsIndividual); } } if(parsedMaterialPromises.length>0){ const parsedMaterialsArray= await Promise.all(parsedMaterialPromises); const parsedMaterials = Object.assign({}, ...parsedMaterialsArray); await parseObj(model, lines, parsedMaterials); } }catch (error) { if (failureCallback) { failureCallback(error); } else { p5._friendlyError('Error during parsing: ' + error.message); } } finally{ if (normalize) { model.normalize(); } self._decrementPreload(); if (typeof successCallback === 'function') { successCallback(model); } } }, failureCallback ); } else { p5._friendlyFileLoadError(3, path); if (failureCallback) { failureCallback(); } else { p5._friendlyError( 'Sorry, the file type is invalid. Only OBJ and STL files are supported.' ); } } return model; };

now single mtl file' materials are fully loaded first and then only passed to parseObj but for multiple files, materials are still being passed before fully loading thus the error: p5.js says: Error during parsing: Cannot read properties of undefined (reading 'diffuseColor')

In the earlier approach even after adding resolve, reject back and some other changes too, still the parseObj was called beforehand.

where is the logic breaking in the current code that single mtl files are passed after materials fully loading but not multiple mtl files? I'm trying to still find since I used Promise.all also.

i tried using a counter to ensure parseObj is called after parsedMaterials are loaded like this:

const parsedMaterialsIndividual = await parseMtl(self, mtlPath); mtlPaths.push(mtlPath); mtlfileno++; parsedMaterialPromises.push(parsedMaterialsIndividual);

and then,

const parsedMaterials = await Object.assign({}, ...parsedMaterialsArray); console.log(parsedMaterials); if(mtlfileno===mtlPaths.length){ parseObj(model, lines, parsedMaterials);

A few little things:

Right now you do const parsedMaterialsIndividual = await parseMtl(self, mtlPath);. By awaiting this, this means synchronously waiting for the material to be loaded/parsed, and the returned value is the actual result and not a promise. This works, but it means all mtl files are loaded one after another, and the Promise.all has no effect due to the results not being promises any more. If you remove the await on this line, the result is a promise, and the code only waits for them to finish at the Promise.all, meaning they all get loaded in parallel, which seems more like what you want.

It sounds like right now, awaiting parseMtl doesn't fully wait for it to be loaded. What does your parseMtl implementation look like right now? In the first comment in this chain, I had added some suggestions on how to update parseMtl to await for loading to complete, have you tried any of that out yet?

Let me know if I can help elaborate on any of those points!

@davepagurek below is the parseMtl function:
you had suggested using doneLoading and this is how I'm currently implementing the function:

function parseMtl(p5,mtlPath){ //accepts mtlPath to load file return new Promise((resolve, reject)=>{ let currentMaterial = null; let materials= {}; p5.loadStrings( mtlPath, lines => { for (let line = 0; line < lines.length; ++line){ const tokens = lines[line].trim().split(/\s+/); if(tokens[0] === 'newmtl') { const materialName = tokens[1]; currentMaterial = materialName; materials[currentMaterial] = {}; }else if (tokens[0] === 'Kd'){ //Diffuse color materials[currentMaterial].diffuseColor = [ parseFloat(tokens[1]), parseFloat(tokens[2]), parseFloat(tokens[3]) ]; } else if (tokens[0] === 'Ka'){ //Ambient Color materials[currentMaterial].ambientColor = [ parseFloat(tokens[1]), parseFloat(tokens[2]), parseFloat(tokens[3]) ]; }else if (tokens[0] === 'Ks'){ //Specular color materials[currentMaterial].specularColor = [ parseFloat(tokens[1]), parseFloat(tokens[2]), parseFloat(tokens[3]) ]; }else if (tokens[0] === 'map_Kd') { //Texture path materials[currentMaterial].texturePath = tokens[1]; } } resolve(materials); },reject ); }); }

…y/p5.js into Mtl-support-to-obj-files

davepagurek · 2024-01-21T18:29:35Z

src/webgl/loading.js

                model.vertexNormals.push(loadedVerts.vn[vertParts[2]].copy());
              }
            }
+            if (usedVerts[vertIndex] && usedVerts[vertIndex]


Right now I think we just store the last used material for each vertex. I think this means that if we reuse the same vertex first with material1, then with material2, then a third time with material1 again, we'll end up with three duplicates of the vertex instead of just two, since we make a new duplicate if the current material is not the same as the last used material.

Maybe instead of storing a map of source index → material, we can store a map from source index → another map of material → destination index. In that world, this if statement would check if usedVerts[vertIndex][currentMaterial] is defined yet, and create it if not. (This would maybe have to also absorb the else branch right above it maybe.) Then, below, we would add usedVerts[vertIndex][currentMaterial] to the current face.

Right, I missed out on how I could have optimized the logic here and thus have lesser duplication like in the example you mentioned above. Will work on it now. Thanks for pointing this out. @davepagurek

davepagurek · 2024-01-21T18:30:12Z

src/webgl/loading.js

    model.computeNormals();
  }
-
+  console.log(model.vertexColors);


I assume this is for debugging, although we should probably take this out before merging.

davepagurek · 2024-01-21T18:31:49Z

src/webgl/loading.js

          ) {
            model.faces.push(face);
+            //same material for all vertices in a particular face
+            const materialDiffuseColor =


Do we need to add a check here if this exists, so that for objs without materials, we can leave the vertexColors array empty?

diyaayay · 2024-02-01T18:44:07Z

@davepagurek Made a few changes in the duplication of vertices and the number of vertices in the model.vertices is lesser , only copying the vertex a minimum number of times as per the requirement, I think?

davepagurek

Nice work on this! I think we're getting really close.

davepagurek · 2024-02-01T19:02:42Z

src/webgl/loading.js

-              }
-              const duplicatedVertIndex = model.vertices.length;
+
+            if (usedVerts[vertParts[0]][currentMaterial] === undefined) {


davepagurek · 2024-02-01T19:04:37Z

src/webgl/loading.js

-                materialDiffuseColor[1],
-                materialDiffuseColor[2]
-              ]);
+            if (currentMaterial && materials[currentMaterial]) {


Right now this will break if we have some vertices that don't have a material but others that do (since it means the earlier vertices won't have pushed colors to the array.) Our data needs it to be all or nothing.

I don't think we need to actually support that case, but maybe we can try to detect it and throw an error if we encounter it? i.e. if we've added any vertices with no colors, and then find ourselves trying to add a vertex with color later on, throw an error letting people know they need colors for all their vertices or none.

davepagurek · 2024-02-01T19:05:29Z

test/unit/visual/cases/webgl.js

+    visualTest('OBJ model with MTL file displays diffuse colors correctly', function(p5, screenshot) {
+      return new Promise(resolve => {
+        p5.createCanvas(50, 50, p5.WEBGL);
+        p5.loadModel('unit/assets/octa-color.obj', model => {


Right now the screenshot looks like just a dot. Maybe to make this clearer we can call model.normalize() to normalize its size to, I think, 200px?

Maybe we can also add one more test importing an object with no colors, and then drawing it with a colored fill() to show that it can take on new colors.

diyaayay · 2024-02-02T13:15:43Z

@davepagurek visual test passes in the terminal but on localhost still shows some discrepancy.

…files

diyaayay · 2024-02-04T15:22:25Z

@davepagurek Let me know what you think, what I've tried to do is for eg: if there are 6 unique vertices at 6 coordinate positions and all of them have color at least once, the model is valid. If none of them has color, the model is valid. Otherwise, throw an error for inconsistency.

src/webgl/loading.js

davepagurek · 2024-02-05T21:20:40Z

Great work on this so far! I think we're almost done -- I left some comments about a possible simplification, and a test for the error case. The last thing I can think of is, how should we handle it if e.g. you export an obj + mtl from Blender but only upload the obj? Right now we throw an error, but maybe it would be better to log a warning without actually throwing an error, and then treat it like it just has no materials? (Would that just involve parsing the obj in the catch block with no materials instead of rejecting the promise or would it be more complicated?)

…files

diyaayay · 2024-02-12T17:44:49Z

@davepagurek Have a look when you find the time. Let me know what else should be done, Thanks.

davepagurek · 2024-02-13T12:30:23Z

src/webgl/loading.js

+            parseObj(model, lines, parsedMaterials);
          }
        }catch (error) {
+          console.error(error.message);


Do we need to console.error here when we _friendlyError below?

Ah yes! I thought of it but was only checking all errors in console while making this work. Sorry for the oversight while pushing. I'll make the changes.

davepagurek · 2024-02-13T12:35:09Z

src/webgl/loading.js

+          }
+          try {
+            if(await fileExists(mtlPath)){
+              const parsedMaterialsIndividual = await parseMtl(self, mtlPath);


By adding await here, we're waiting for each material to load one at a time rather than waiting for all of them to load in parallel at the Promise.all below on line 195. Does this still work if we take out the await on this line, given that we already wait for all of them later?

Removing await here would result in parallel loading of all mtl filled and then await with promise.all which is more efficient in handling all files together , better than sequential parsing of each mtl file.

davepagurek · 2024-02-13T12:39:34Z

src/webgl/loading.js

+            mtlPath = mtlFilename;
+          }
+          try {
+            if(await fileExists(mtlPath)){


This await will also cause us to do these HEAD requests one at a time instead of parallel. Maybe we can do something like:

parsedMaterialPromises.push( fileExists(mtlPath).then(exists => { if (exists) { return parseMtl(self, mtlPath); } else { console.warn('MTL file not found or error in parsing; proceeding without materials.'); return {}; } }).catch(error => { console.warn('Error loading MTL file:', error); return {}; }) );

(Also, while we're at it, maybe we can add the URL for the mtl file that we couldn't find to the warning we log?)

davepagurek · 2024-02-13T12:41:29Z

src/webgl/loading.js

+  }
+  async function fileExists(url) {
+    try {
+      const response = await fetch(url, { method: 'HEAD' });


diyaayay · 2024-02-14T06:51:46Z

@davepagurek Does this seem better? Thanks for the suggestions.

davepagurek

Thanks for your hard work on this and all the iterations you've made! I think this is good to go now!

Right now we're in the middle of releasing v1.9.1, and have a beta build that we're bug testing, so we're just merging in bug fixes until we finish the release. I've approved this PR though, so as soon as we finish the release, I'll merge this in!

diyaayay · 2024-02-14T21:27:57Z

@davepagurek Had a great time working on this, as a university student this PR improved my vanilla javascript concepts a lot. Thank you for putting in all the time and reviewing it so many times✨

Qianqianye · 2024-03-19T20:12:10Z

Looks great! Thanks @diyaayay and @davepagurek!

diyaayay added 2 commits January 8, 2024 03:08

fixed unit test

a47fbbc

removed console logs

2143dba

diyaayay and others added 8 commits January 10, 2024 01:11

changed calculation of diffuse to vertex colors

3498ad0

Merge branch 'processing:main' into Mtl-support-to-obj-files

5c7c07e

removed console logs, changed default vertexColors to 1

b46af55

Merge branch 'Mtl-support-to-obj-files' of https://github.com/diyaaya…

0b7a1fe

…y/p5.js into Mtl-support-to-obj-files

Fixes vertexColor and mtlpath assignment

77637ba

remove console logs

1169993

added unit test for loading obj mtl and assigned vertex colors

06c5e28

unit test for missing mtl file

2b46a55

davepagurek reviewed Jan 11, 2024

View reviewed changes

diyaayay and others added 5 commits January 13, 2024 12:18

unit tests for later when vertexColors are properly assigned

0790146

plant.obj to smaller file for testing

1fa08a2

added multiple mtl file handling, still need to fix vertexColors

be5cf6c

handles other failing unit tests

cd6039d

Merge branch 'processing:main' into Mtl-support-to-obj-files

32f94f3

davepagurek reviewed Jan 14, 2024

View reviewed changes

davepagurek mentioned this pull request Jan 18, 2024

Defaults for buildGeometry #6722

Open

17 tasks

diyaayay and others added 6 commits January 19, 2024 19:39

vertex color assign and duplicate vertices

16fad39

Merge branch 'Mtl-support-to-obj-files' of https://github.com/diyaaya…

c3538b7

…y/p5.js into Mtl-support-to-obj-files

Merge branch 'main' into Mtl-support-to-obj-files

3cb1a13

commit before merging

90a3a53

linting erros , conflicts

2fffa53

had merge conflicts

48b07a3

davepagurek reviewed Jan 21, 2024

View reviewed changes

removes console logs

7236340

davepagurek reviewed Feb 1, 2024

View reviewed changes

normalize model for visual test

b32c935

diyaayay added 3 commits February 2, 2024 22:45

Merge remote-tracking branch 'upstream/main' into Mtl-support-to-obj-…

19e4655

…files

visual tests work for fill in .obj model and mtl color png fix

14a2420

adds check for either a fully colored model or colorless model

687e010

davepagurek reviewed Feb 5, 2024

View reviewed changes

src/webgl/loading.js Show resolved Hide resolved

src/webgl/loading.js Outdated Show resolved Hide resolved

diyaayay added 4 commits February 8, 2024 23:00

Merge remote-tracking branch 'upstream/main' into Mtl-support-to-obj-…

2aa937f

…files

colored model and unit test for color-no color

31bdeb9

mtl file not found, obj displayed without materials

ac68110

unit test for missing mtl displayes obj file without color

d7aa2f7

davepagurek reviewed Feb 13, 2024

View reviewed changes

Sequential to Parallel mtl file handling

25bbdc9

davepagurek approved these changes Feb 14, 2024

View reviewed changes

davepagurek mentioned this pull request Feb 22, 2024

1.9.1 Release #6822

Closed

3 tasks

diyaayay mentioned this pull request Feb 23, 2024

GSoC: JSON Schema Language Server Contributions json-schema-org/community#601

Closed

Qianqianye merged commit 316a5c0 into processing:main Mar 19, 2024

This was referenced Mar 24, 2024

Textures coordinates are incorrectly applied on .obj models #6921

Closed

Enable .mtl File Uploads processing/p5.js-web-editor#3067

Closed

Support for .mtl Files with Textures #6924

Open

diyaayay mentioned this pull request Aug 13, 2024

Support for .mtl Files with Textures #7072

Open

3 tasks

diyaayay deleted the Mtl-support-to-obj-files branch November 13, 2024 18:11

Uh oh!

Comments

Conversation

diyaayay commented Jan 7, 2024

PR Checklist

Uh oh!

diyaayay commented Jan 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

diyaayay commented Jan 7, 2024

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diyaayay Jan 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davepagurek commented Jan 14, 2024

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diyaayay Jan 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diyaayay commented Feb 1, 2024

Uh oh!

davepagurek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diyaayay commented Jan 7, 2024 •

edited

Loading

diyaayay Jan 11, 2024 •

edited

Loading

diyaayay Jan 15, 2024 •

edited

Loading

diyaayay commented Feb 4, 2024 •

edited

Loading

diyaayay Feb 13, 2024 •

edited

Loading