Add independent loadModel() U and V flipping flags

[myselfhimself]

Hello,

How would this new feature help increase access to p5.js?

The feature suggests adding extra parameters to loadModel() so that OBJ files can have their U and/or V texture coordinates flipped, in order to fine-tune flipped texture displays.

Most appropriate sub-area of p5.js?

• Accessibility (Web Accessibility)
• Build tools and processes
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Friendly error system
• Image
• IO (Input/Output)
• Localization
• Math
• Unit Testing
• Typography
• Utilities
• WebGL
• Other (specify if possible)

Feature enhancement details:

In issue #4866 and related PR #5017, we fixed p5js's OBJ vertex UV texture coordinates parsing's behaviour to more closely match that of Processing, where the V coordinate is inverted, to anticipate WebGL's flipping of that V/Y coordinate. Still, this does not solve all texture horizontal or vertical flipping issues for people exporting .OBJ files for a p5js import using many kinds of 3d software.

Diverging slightly from Processing's loadShape() (which tolerates .OBJ files to make 3d geometries)'s signature, why not add 2 parameters to the p5js loadModel(modelPath:String, normalize:boolean), giving one of those possibilities:

• loadModel(modelPath:String, normalize:boolean, flipU:boolean, flipV:boolean)
• loadModel(modelPath:String, normalize:boolean, flipUV:list) where flipUV would be a list of two 0-1 integers, eg. [0,1] for U and V respectively
• .. open to more ideas

Thanks @stalgiag for pushing me into adding this new issue

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, be sure to follow the issue template if you haven't already.

[myselfhimself]

I would go for an configuration object as to the flipU, flipV additional parameter, similar to the options one in:
https://p5js.org/reference/#/p5.Font/textToPoints

 options
Object:

an (optional) object that can contain:


sampleFactor - the ratio of path-length to number of samples (default=.1); higher values yield more points and are therefore more precise


simplifyThreshold - if set to a non-zero value, collinear points will be be removed from the polygon; the value represents the threshold angle to use when determining whether two edges are collinear (Optional)

Such a key-value object helps adding other fancy options for the future without always increasing loadModel()'s parameters number. That other objects may also be a sort of symbol for options that do not originate from Processing.

[stalgiag]

Thanks @myselfhimself! I agree that an object literal parameter would be a good idea here. I can see more parameters being added to loadModel() in the future and there are already quite a few. I wonder if fileType should be part of the options parameter.

[myselfhimself]

Thanks for commenting Indeed fileType could force the parser to interpret some file without extension to be detected as something specific (obj, stl..), such as a blob downloaded or from some remote server or badly named.. I wonder if that should be dealt with in a different issue or this one; the amount of code may not be that big though for it, so we might want to rename the present issue to "Add 'options' object parameter to loadModel() for U,V flipping, fileType". What do you think? Not being a native english speaker, what would be more natural for UV flipping keyword parameters: flipU:bool flipV:bool or invertU:bool invertV:bool ? út 2. 2. 2021 v 16:32 odesílatel Stalgia Grigg <[email protected]> napsal:

…

Thanks @myselfhimself <https://github.com/myselfhimself>! I agree that an object literal parameter would be a good idea here. I can see more parameters being added to loadModel() in the future and there are already quite a few. I wonder if fileType should be part of the options parameter. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5021 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJU5QXO2225Q6FJMM6OA43S5ALJPANCNFSM4WY7DHAA> .

[deveshidwivedi]

Hi! Is this issue still open to work on?

[myselfhimself]

Hello I haven't implemented the idea. It could be done by anyone. Le mer. 13 déc. 2023, 16:39, Deveshi Dwivedi ***@***.***> a écrit :

…

Hi! Is this issue still open to work on? — Reply to this email directly, view it on GitHub <#5021 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJU5QUIMBZL6SM6LMVCQP3YJHD2HAVCNFSM4WY7DHAKU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TCOBVGQYTMNBSHA2Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[deveshidwivedi]

I'd like to take this up please. Could you please guide me how to go about it?
I will read more and get back here!

[deveshidwivedi]

Hi! I think in loading.js, we could introduce the parameters as:

/**
 * Load a 3D model with optional UV flipping.
 * @method loadModel
 * @param {String} path 
 * @param {function(p5.Geometry)} [successCallback] 
 * @param {function(Event)} [failureCallback]
 * @param {String} [fileType] 
 * @param {Object} [options] 
 * @param {boolean} [options.normalize] 
 * @param {boolean} [options.flipU] 
 * @param {boolean} [options.flipV] 
 * @return {p5.Geometry} the <a href="#/p5.Geometry">p5.Geometry</a> object
 */

Introducing additional parameters to the loadModel function to allow for more flexibility and customization:

p5.prototype.loadModel = function(path, successCallback, failureCallback, fileType, options)

Declaration of the parameters:

  let normalize = false;
  let flipU = false;
  let flipV = false;
  
  
  if (options && typeof options === 'object') {
    normalize = options.normalize || false;
    flipU = options.flipU || false;
    flipV = options.flipV || false;
  }

Modifying loaded model for U V flipping:

 if (fileType.match(/\.obj$/i)) {
    this.loadStrings(
      path,
      strings => {
        parseObj(model, strings, { flipU, flipV });

        if (normalize) {
          model.normalize();
        }

        self._decrementPreload();
        if (typeof successCallback === 'function') {
          successCallback(model);
        }
      },
      failureCallback
    );

Now, the function accepts an additional options parameter.
The UV flipping options (flipU and flipV) are extracted from the options object. When calling parseObj, the UV flipping options are passed to the parsing function. Before parsing the OBJ file using loadStrings, we added logic to check if the options object is provided. If so, the flipU and flipV values from the options object are passed to the parseObj function, enabling the customization of UV flipping behavior.
How does this look?

[deveshidwivedi]

@davepagurek I'd request you to give your views as well!

[davepagurek]

Thanks for your interest @deveshidwivedi! A few thoughts:

• Maybe we can try to keep the existing method signatures (loadModel(path, normalize, [successCallback], [failureCallback], [fileType]) and loadModel(path, [successCallback], [failureCallback], [fileType])) but introduce a third new one, which is just loadModel(path, [options])? The options object can include optional parameters for all the options available in the other method signatures, allowing you to only specify options you need, regardless of order.
• I'm not 100% sure if @param {boolean} [options.flipV] correctly links the type to the property of the object. @limzykenneth do you know if that syntax is correct or not? createFramebuffer's docs just list the options in a bullet list outside of the types.
• How do you feel about adding a flipU() and flipV() method to p5.Geometry? That way you can call it on models after they've been loaded like this, and hypothetically you could also do it on code-generated models too:

  let myModel
  function preload() {
    myModel = loadModel('model.obj')
  }
  function setup() {
    myModel.flipU()
    myModel.flipV()
  }

[limzykenneth]

@param {boolean} [options.flipV] syntax should work with both JSDoc and YUIDoc, how it renders however I'm not sure. It might not render the parameter properties and just render the parameter object but need to try to be sure.

[deveshidwivedi]

Thank you!@davepagurek
Currently, the function checks the type of the second argument (arguments[1]) to determine which signature is being used. If the second argument is a boolean, it assumes the signature is loadModel(path, normalize, [successCallback], [failureCallback], [fileType]) and otherwise it assumes the signature is loadModel(path, [successCallback], [failureCallback], [fileType]).
To introduce a third one, should we modify the if-else loop to be something like:

 if (typeof arguments[1] === 'object') {
    options = arguments[1];
  } else {
 
 options = {
    normalize: arguments[1],
    successCallback: arguments[2],
    failureCallback: arguments[3]
    };

    if (typeof arguments[4] !== 'undefined') {
      fileType = arguments[4];
    }
  }

Could you give an idea as to what the flipU and flipV models would contain?

[davepagurek]

@deveshidwivedi that code snippet looks like a decent way to go about it!

Could you give an idea as to what the flipU and flipV models would contain?

I'm imagining something sort of like this:

p5.Geometry = class Geometry {

  // ...
  // (exisiting methods are in here)
  // ...

  flipU() {
    this.uvs = this.uvs.flat().map((val, index) => {
      if (index % 2 === 0) {
        return 1 - val;
      } else {
        return val;
      }
    });
  }
}

...and similar for v but where you flip the other ones. The .flat() part is I think necessary because some parts of the code treat uvs like an array of arrays like [[u,v], [u,v], [u,v]] while other parts treat it like a flat array like [u,v,u,v,u,v]. (Alternatively, maybe we can avoid the .flat() here by making the data type consistent?)

[deveshidwivedi]

@davepagurek I get it now:) I was wondering if we could pass the coordinates as array of pairs to avoid inconsistency.
this.uvs, would directly flip the u or v coordinate based on its position in the pair. Something like this:

p5.Geometry = class Geometry {
//.....
flipU() {
  this.uvs = this.uvs.map(pair => [1 - pair[0], pair[1]]);
  return this; 
}

flipV() {
  this.uvs = this.uvs.map(pair => [pair[0], 1 - pair[1]]);
  return this; 
}
}

Does this seem okay?

[davepagurek]

Avoiding inconsistency would be great! Just a question of whether to standardize on a flat array or on an array of pairs.

Right now I think it might be a slightly better choice to standardize on a flat array? It looks like that's how most code in p5 treats it already (some exceptions in some shapes in webgl/3d_primitives.js, and for loaded models in webgl/loading.js) and arrays need to be flat anyway when passed into vertex buffers for the native WebGL APIs, so it would be a bit more efficient to store them in that format from the start. Can you think of any drawbacks of that format?

[deveshidwivedi]

Alright, I understand that standardizing on a flat array would be better. Thank you!

While trying to introduce loadModel(path, [options]), I tried running npm test and came across these errors, should I try to optimize the code so that it does not exceed the time limit?

[davepagurek]

Do those tests fail for you on the main branch as well or are those introduced by the changes? If this makes things slower, then I think it makes sense to take a look into why it's taking a while. If it's like that on main too, making things faster is always nice, but don't feel like you need to do that as part of this change. For now, you can try manually changing the timeout for these tests so that you can check that they run correctly, and then put it back to the default timeout before submitting a PR.

[deveshidwivedi]

The tests failed due to the changes introduced. I tried to make it better but it still needs improvement. There are still some errors due to loading.js, could you please have a look? @davepagurek
updated loadModel()

[davepagurek]

Interesting, I get some different errors when I run the code. The first set of errors were due to some trailing spaces at the ends of lines, and once I edited those out, instead of getting timeout errors, I get some errors about values not matching and a callback not being defined. It looks like that might be because, in the else branch, all the arguments are now shifted over one index from what they used to be:

I think this is because the case when arguments[1] is a boolean is no longer handled. It's unfortunate that to be fully backwards compatible we now need three separate branches instead of just two, but I think we might need that for now.

[deveshidwivedi]

I'm sorry for being unclear, the errors I was getting were different than timeout errors after I made changes.

I understand where the problem is. I will try to work on this bit to see that all the three cases are handled. Thank you!

[davepagurek]

no problem, good luck!

[deveshidwivedi]

Hi! @davepagurek
Here are the changes made. There are no errors anymore and all tests successfully pass. Please have a look.
Here's how the three cases are handled:

• If the second argument is an object, it assumes that the function is called with the third signature: loadModel(path, [options]). It extracts optional parameters (normalize, successCallback, failureCallback) from the provided options object.
• If the second argument is not an object, it checks if the function is called with the first or second signature. If the second argument is a function, it assumes the second signature: loadModel(path, normalize, [successCallback], [failureCallback], [fileType]). It extracts successCallback and failureCallback from the arguments. If the fourth argument exists, it is assumed to be fileType.
• If the second argument is not a function, it assumes the first signature: loadModel(path, [successCallback], [failureCallback], [fileType]). It extracts successCallback and failureCallback from the arguments. If the third argument exists, it is assumed to be fileType.

[davepagurek]

Thanks for the updates, it's looking good!

It looks like in the else branch, it doesn't yet handle the case with the parameters being path, normalize, [success], [failure], since it looks like it doesn't assign to normalize if arguments[1] isn't a function.

The other thing we might want to change is, instead of editing the parameters in the doc comments (since we want that version of the API to still work), we can add an additional doc comment with the new function signature that uses options. If you scroll up above the start of the function, you'll notice the @method loadModel line appears twice, and each of those doc comment blocks contributes a line to this part of the docs:

I think we want to leave those existing two lines alone, but add a third variant by adding a new doc comment.

Also, feel free to open a PR with the code you have! It looks like it's almost ready, and might be easier to review once it's in a PR.