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

Long time spent in _edgesToVertices when assembling shapes into composite models #7237

Closed
2 of 17 tasks
WorldSEnder opened this issue Sep 5, 2024 · 2 comments · Fixed by #7287
Closed
2 of 17 tasks

Comments

@WorldSEnder
Copy link

WorldSEnder commented Sep 5, 2024

Most appropriate sub-area of p5.js?

  • Accessibility
  • Color
  • Core/Environment/Rendering
  • Data
  • DOM
  • Events
  • Image
  • IO
  • Math
  • Typography
  • Utilities
  • WebGL
  • Build process
  • Unit testing
  • Internationalization
  • Friendly errors
  • Other (specify if possible)

p5.js version

v1.10.0

Web browser and version

Firefox 128.0.3, Chrome 128.0.6613.84

Operating system

Any

Steps to reproduce this

I am assembling my "world" into geometry, updating it whenever the world changes. Since updates are very rare, this should give me a nice boost in performance. I noticed that whenever the world does change, and I have to refresh the prebuilt geometry, a lot of time is spent in _edgesToVertices (up to 88% when first drawing the new geometry). While it does make sense to me that high-fidelity strokes cost performance and need this step, I do not see why it needs to go through this step in the first place, given that I assemble my geometry from primitives that already should have, or should have cached, their strokes.

EDIT: The above is compounded by the fact that this step is apparently done even when I explicitly render the model with noStroke() (during [begin|end]Geometry and when rendering with model(..)) meaning I can not find a way to work around this post processing step.

Steps:

Snippet:

const TSZ = 50; // Tilesize
const BLH = 30; // Block height
const FLH = 10; // Floor height

this.hasFloor: boolean[] = [....];

// I do the following every time hasFloor changes
p5.beginGeometry();
for (let x = 0; x < this.length; x++) {
    for (let y = 0; y < this.width; y++) {
        if (!this.hasFloor[x * this.width + y])
            continue;
        p5.push();
        p5.translate(x * TSZ, y * TSZ, (-BLH - FLH) * 0.5);
        p5.box(TSZ, TSZ, FLH);
        p5.pop();
    }
}
const floorModel = p5.endGeometry();
// ...I do the following every draw call
p5.model(floorModel);
@WorldSEnder WorldSEnder added the Bug label Sep 5, 2024
Copy link

welcome bot commented Sep 5, 2024

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms. Thank you!

@davepagurek
Copy link
Contributor

Two points here:

For why stuff without strokes ends up spending time on stroke calculations anyway: it looks like when we draw a model, we generate strokes around every triangle if we have no stroke info:
https:/processing/p5.js/blob/main/src%2Fwebgl%2Floading.js#L1121-L1123

I suspect this was so that something like sphere() wouldn't have to manually add edges everywhere. But we could add a flag to objects that come from buildGeometry and skip that check if we see that flag is set.

For why it has to recalculate anything when combining shapes: I think we could be adding calculated edge data like lineVertices when adding a shape in GeometryBuilder!

If anyone's interested in taking on any of these tasks before I get to them, let me know, I'm happy to help review!

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

Successfully merging a pull request may close this issue.

2 participants