-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Adds geometry to downloadable obj files functionality #6812
base: main
Are you sure you want to change the base?
Conversation
@davepagurek Please take a look and let me know if this works and if further changes are needed, maybe unit tests, examples, or even how I've implemented this. Thanks. |
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's looking good so far! Some user-facing documentation would be great.
For unit tests, some of our tests use this method to test downloads, maybe we can try using it too?
Line 27 in ef4647f
function testWithDownload(name, fn, asyncFn = false) { |
an example of its usage:
p5.js/test/unit/io/saveTable.js
Lines 99 to 112 in ef4647f
testWithDownload( | |
'should download a file with expected contents (tsv)', | |
async function(blobContainer) { | |
myp5.saveTable(myTable, 'filename', 'tsv'); | |
let myBlob = blobContainer.blob; | |
let text = await myBlob.text(); | |
let myTableStr = myTable.columns.join('\t') + '\n'; | |
for (let i = 0; i < myTable.rows.length; i++) { | |
myTableStr += myTable.rows[i].arr.join('\t') + '\n'; | |
} | |
assert.strictEqual(text, myTableStr); | |
}, | |
true | |
); |
|
||
|
||
// Vertices | ||
this.vertices.forEach(v => { |
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.
Nice work on this method, nice and concise!
src/webgl/p5.Geometry.js
Outdated
objStr += `f ${faceStr}\n`; | ||
}); | ||
|
||
save(objStr, fileName); |
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.
save()
is a user-facing function, so if this method uses it, users could also technically use it, but they would have to know to pass an obj string through here. Maybe instead we could use the internal downloadFile
method?
Line 227 in 37d3324
p5.prototype.downloadFile(blob, filename, extension); |
src/webgl/p5.Geometry.js
Outdated
* @for p5.Geometry | ||
*/ | ||
|
||
geometryToObj (fileName='model.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.
Maybe we can call this saveObj
so users would call it like geom.saveObj()
instead of geom.geometryToObj()
, which feels a tad redundant?
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.
saveObj
sounds much better :)
@davepagurek made some changes, let me know if these work.Thanks. |
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.
Great work on the writing for the documentation! I just have a few things left for the example.
I also thought of one more thing for the exporter itself about handling cases where we don't have either normals or texture coordinates.
* }); | ||
* | ||
* octa = endGeometry(); | ||
* octa.saveObj(); |
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.
Can we do this on button click maybe? I think going to the reference page and immediately having the file immediately download might be a bit jarring.
* let angleX = 0; | ||
* let angleY = 0; | ||
* function setup() { | ||
* createCanvas(400, 400, WEBGL); |
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.
// Faces | ||
this.faces.forEach(face => { | ||
// OBJ format uses 1-based indices | ||
const faceStr = face.map(index => index + 1).join(' '); |
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.
If we have normals and texture coordinates, I think we might have to use f 1/1/1 2/2/2 3/3/3
to specify that the face does in fact have vertices and faces? We read it out from the slashes here:
Line 252 in 37d3324
const vertParts = vertString.split('/'); |
It might take a few if statements to do e.g. f 1//1 2//2 3//3
if we have normals but no texture coordinates.
Resolves #6769
Changes:
1- Added obj file support to
save()
.2- Created a method called
geometryToObj()
inp5.Geometry.js
.Screenshots of the change:
Sketch used:
Downloaded Obj File:
Sketch to check the downloaded obj file:
Downloaded obj file on canvas:
PR Checklist
npm run lint
passes