Skip to content

RFC: Geometry3D refactor #1056

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

Closed
indefinit opened this issue Oct 31, 2015 · 10 comments
Closed

RFC: Geometry3D refactor #1056

indefinit opened this issue Oct 31, 2015 · 10 comments

Comments

@indefinit
Copy link
Contributor

While patching #1009 and paving the way for #904, I found that Geometry3D could benefit from a refactor. As such, I wanted to post my thinking here, and request feedback in case anyone has a strong opinion, either for or against the refactor.

Currently, when a user calls sphere() the geometry and buffers are created and added to respective hash tables. This is what we've labeled "retainedMode". It's part of the secret sauce behind the performant, non-scene-graph style architecture in p5.js. When a user calls line() or quad() in WEBGL mode, however, their 2D geometries are created directly as webgl array buffer data, and consequently are not entered into the geometry hash table. We've labeled this implementation approach as "immediateMode". While I do see many strengths in an immediateMode approach (namely begin and endShape()), I don't think we need to have all 2D drawing be done in this way. The benefits of consolidating geometry in webgl include a performance boost and less error prone source code.

With a Geometry3D refactor, both 3D and 2D geometry primitives would be created similarly, each in "retainedMode". Because the Geometry3D class would consequently contain both 3D and "2D" primitives, we'd rename the class to simply "p5.Geometry", and 3d_primitives.js would become primitives.js.

Objections or encouragements? tagging:
@karenpeng @codeanticode @lmccart @shiffman @mindofmatthew
but comments from others are welcome.

@codeanticode
Copy link
Contributor

I think it makes sense to unify the underlying handling of 2D and 3D geometry. So this means that objects generated with begin/endShape() will still be drawn in immediate mode?

@indefinit
Copy link
Contributor Author

@codeanticode yes, begin/endShape will still utilize immediate mode, but default primitives (2D and 3D) will otherwise use retainedMode.

@karenpeng
Copy link
Member

Upvote for unifying 2D and 3D primitives. They work in similar ways and have similar use case. Actually I've been thinking instead of drawing a line, draw a tube geometry, in this way we have better control with the stroke width.

@indefinit
Copy link
Contributor Author

Hi All,

Sorry for the radio silence these last couple weeks. I've dived into a pretty large refactor which is nearing completion. There's still a couple of bugs I'm working out before merging into my master branch but if you're curious to see the progress, take a look here: https://github.com/indefinit/p5.js/tree/geometry-refactor

Some highlights from the changes:

  • Geometry is more scalable in anticipation of a future 3D model loader library or just more complex generative meshes.

  • Reducing the number of Webgl buffers. For performance reasons, it's better to have larger but fewer webgl buffers.

  • simpler internal api for developers looking to extend and build on existing core p5 webgl:
    to create a custom retainedMode geometry, a user would do something like this:

    
    var customGeom = new p5.Geometry(func, detailX, detailY);
    this._renderer.createBuffer(gId, customGeom);
    
    

    where func is either a geometric function used to generate vertices or an object literal containing p5.Vector's as mesh vertices. gId is the unique ID for retrieving the buffered geometry from our Geometry Hash Map.

You'll also notice a few of the files have been renamed inside src/3d/ . After the above changes are finalized and merged in with the master branch, I think we should rename this directory as webgl primarily because webgl mode is now becoming dimensionally agnostic. Renderer3D will therefore rename to RendererGL.

Also included in the refactor is a more feature rich ImmediateMode. Fills and pointSize values now work as expected in ImmediateMode (aka beginShape / endShape), and we now support all of the Webgl drawing primitives. They are:

  • POINTS
  • LINES
  • LINE_LOOP
  • LINE_STRIP
  • TRIANGLES
  • TRIANGLE_STRIP
  • TRIANGLE_FAN

Take a look at the code (it's still a work in progress, so a little buggy). Any questions, comments, feature suggestions, nomenclature are much appreciated! Thanks.

@codeanticode
Copy link
Contributor

This is really exciting! I will take a look as soon as I have a chance. Are you planning to introduce custom shaders anytime soon, or that's an addition for a later time?

@indefinit
Copy link
Contributor Author

Custom shaders are definitely in the pipeline! Not necessarily with this
refactor but hopefully that'll be my next focus post the geometry updates.

On Mon, Nov 16, 2015 at 12:28 PM, codeanticode [email protected]
wrote:

This is really exciting! I will take a look as soon as I have a chance.
Are you planning to introduce custom shaders anytime soon, or that's an
addition for a later time?


Reply to this email directly or view it on GitHub
#1056 (comment).

Kevin Siwoff
// http://kevinsiwoff.com // Instagram: @kevinsiwoff
http://instagram.com/kevinsiwoff/ //

@indefinit
Copy link
Contributor Author

Had a good conversation offline at today's p5.js meetup with @mindofmatthew and @polyrhythmatic about paving the way for the obj loader. Two notable improvements to be made to the p5.Geometry object:

  • Separate out normals/uv calculation from object initialization, so that user can choose to include one's own normals/uv as data. Current behavior defaults to auto calculating normals/uvs.
  • Current UV data structure/calculation is error-prone. Consider a per vertex UVs implementation instead of per-Face.

@indefinit
Copy link
Contributor Author

Bringing this conversation back since I want to get this PR merged into master as soon as possible so I can stop holding it hostage from other contributors. I've created a new branch in this repo for the geometry-refactor. I think it should help eliminate duplication of efforts since we now have a bunch of webgl contributors (yay!) and can close issues faster. The branch is currently located called webgl-geometry-refactor

adding @joecridge and @parsoyaarihant to this thread to keep everyone in the loop. Thanks for your patience on this!

@lmccart
Copy link
Member

lmccart commented Jan 1, 2016

👍

@indefinit
Copy link
Contributor Author

Closing this thread since Geometry is indeed refactored in the geometry-refactor branch. I'm just fixing one issue related to vertexNormals and then it is ready to merge into master. Thank you for your comments/input!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants