Skip to content
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

Solves issue #6891 #7165

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Solves issue #6891 #7165

merged 3 commits into from
Aug 20, 2024

Conversation

Garima3110
Copy link
Member

Resolves #6891

Changes:
This is a continuation of the work initiated by @mathewpan2 in the PR #6936

I would love to have further suggestions to the work.
Thankyou!

PR Checklist

@Qianqianye Qianqianye requested a review from davepagurek August 16, 2024 23:07
Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting the tests passing on this! Just some comments on the documentation, and then this looks good!

/**
* 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>.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need this sentence, since loading from a string means we don't have to asynchronously load the source code and everything happens immediately.

* let octahedron;
*
* function preload() {
* octahedron = createModel(octahedron_model);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to above, we can move this to setup to avoid confusion about how loading works.

*
* function setup() {
* createCanvas(100, 100, WEBGL);
* describe('Vertically rotating 3-d octahedron.');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small update for consistency of language across our docs:

Suggested change
* describe('Vertically rotating 3-d octahedron.');
* describe('Vertically rotating 3D octahedron.');

@davepagurek
Copy link
Contributor

oh do we want to keep in the description in the docs of the options object, since we still allow you to pass in things like flipU?

@Garima3110
Copy link
Member Author

oh do we want to keep in the description in the docs of the options object, since we still allow you to pass in things like flipU?

yeah we should , that was a mistake from my side thanks for noticing that..!

Copy link
Contributor

@davepagurek davepagurek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good!

@davepagurek davepagurek merged commit 0312e44 into processing:main Aug 20, 2024
2 checks passed
@Garima3110 Garima3110 deleted the createModel branch August 21, 2024 05:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow loading of models from strings
2 participants