-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
added createModel() to enable loading stl/obj files from a plaintext string within editor #6936
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking this on, it's looking good so far!
src/webgl/loading.js
Outdated
} | ||
} | ||
const model = new p5.Geometry(); | ||
model.gid = `octa.obj|${normalize}`; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this might lead to conflicting IDs if you make more than one. Can we maybe make a global counter that increments each time this is called so that every object gets a unique gid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah definitely something I missed, took your suggestion and added the global counter. Set up the gid as model.gid = `${fileType}|${normalize}|${modelCounter }`;
, let me know if you think this is clear enough.
src/webgl/loading.js
Outdated
model.normalize(); | ||
} | ||
|
||
if (flipU) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like these steps get called regardless of the type of the model. Maybe we can make the if statement by inside the try
so that we can call flipU()
, flipV()
, and the success callback in one spot instead of duplicating the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to follow the convention of loadModel()
, where they also call those statements twice, but I agree that it's cleaner if it's only called once at the end.
src/webgl/loading.js
Outdated
* rendered without color properties. | ||
* | ||
* Options can include: | ||
* - `path`: Specifies the plain text string of either an stl or obj file to be loaded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than specifying a path
, maybe we can make the fileType
mandatory and place it after the source string for all the different overloads for this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the filetype to the argument and fixed some of the overloads, let me know if you like it more now.
…-loading-from-string
/** | ||
* Load a 3d model from an OBJ or STL string. | ||
* | ||
* <a href="#/p5/loadModel">createModel()</a> should be placed inside of <a href="#/p5/preload">preload()</a>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can probably take this sentence out since I think we don't do any preloading in this method?
} | ||
return; | ||
} | ||
} else if (fileType.match('obj')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @return {p5.Geometry} the <a href="#/p5.Geometry">p5.Geometry</a> object | ||
*/ | ||
let modelCounter = 0; | ||
p5.prototype.createModel = function(modelString, fileType, options) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here, try initializing fileType
like this:
p5.prototype.createModel = function(modelString, fileType=' ', options) {
This probably should eliminate the error you're encountering which suggested that the fileType
is undefined when match()
is called on it.
Hii @mathewpan2 , I noticed this PR has been open for a while without any activity. If you're unable to continue working on it, I'd be happy to take over and implement the suggested changes to help solve the issue. Please let me know if that works for you. |
I'm going to close this as @Garima3110 finished it up in another PR. Thanks for your work getting this feature in! |
@all-contributors please add @mathewpan2 for code |
I've put up a pull request to add @mathewpan2! 🎉 |
Resolves #6891
Changes
Added a method called
createModel()
withinsrc/webgl/loading.js
that allows users to call for loading a obj or stl file from a plain text string within the editor. It includes the same options as theloadModel()
function, as well as usingp5._validateParameters('loadModel', arguments);
to check for arguments (I wasn't sure it warranted adding a separate check for 'createModel' since they share the same arguments).Wanted to make sure this implementation was acceptable before adding unit tests.
Screenshots of the change
Given the sampe code above, this successfully generates a rotating 3D model from the string:
PR Checklist
npm run lint
passes