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

Perf: Skip nodes that have no renderable props. #100

Merged
merged 1 commit into from
Dec 20, 2023

Conversation

wouterlucas
Copy link
Contributor

Add color: 0 to the node and don't provide any texture/image properties and it will be skipped in the render step.

@wouterlucas wouterlucas added the performance Performance enhancement label Dec 6, 2023
@chiefcll
Copy link
Contributor

chiefcll commented Dec 7, 2023

What does this solve? Are frameworks creating many nodes that shouldn't be rendered? How many are being skipped?

@wouterlucas
Copy link
Contributor Author

@chiefcll if the node has no renderable properties it will be skipped by the render step.

This will allow SolidJS / JSX to add nodes (e.g. div tags) that do have X / Y / zIndex positioning for its children, but have no "renderable properties" that require to be drawn on the screen.

For example:

<ParentNode x=100 y=200 width=500 height=500 zIndex=2>
  <ChildNode x=10 y=10 src="..image">
  <ChildNode ...>
</ParentNode>

It allows you to freely manipulate the ParentsNode world position/width/etc. without it getting actually rendered. Prior to this change even ParentNode would get a GPU Render Pass, just have an empty instruction so the GPU gets the upload but then figures out it doesn't need to draw anything.

By forcing ParentNode to be color=0 from the App Framework like Solid or Blits, you can have the render still do the positioning but skip the rendering portion. For added clarity, that render step is 80% of the resources spent to render that node. Thus increasing FPS.

This helps a lot by avoiding render steps on nested nodes that do not require to be rendered, but are part of the tree for positioning / zIndex / alpha / container-ish logic.

@chiefcll
Copy link
Contributor

chiefcll commented Dec 7, 2023

Thanks for the details. That makes sense. Right now solid is defaulting to transparent for the color. I assume with this change we should remove setting the color? Or set the color to 0? I’ll play around 🙂

@wouterlucas
Copy link
Contributor Author

I think you’re already setting it to 0x0 in Solid by default if there is no image/texture and if color isnt present. Should be plug and play 😃

@frank-weindel
Copy link
Collaborator

Probably best to make this calculated during update(). As this PR stands I think there are different types of property assignment that will not trigger the calculation to be done.

@wouterlucas
Copy link
Contributor Author

What different properties do you anticipate other then color (and variants), texture (and image), shader and text?

And I don't mind changing it to be part of the update() loop but I suspect it would evaluated more often then just at get/sets of select properties. This is because you're going to run update() every frame, while the renderability wont change that frequent. Hence it being more expensive to evaluate in the update() loop then on set.

@frank-weindel
Copy link
Collaborator

What different properties do you anticipate other then color (and variants), texture (and image), shader and text?

And I don't mind changing it to be part of the update() loop but I suspect it would evaluated more often then just at get/sets of select properties. This is because you're going to run update() every frame, while the renderability wont change that frequent. Hence it being more expensive to evaluate in the update() loop then on set.

As it stands if the texture/shader/text are updated the renderability is not updated along with it. I'm simply proposing the recalculation of renderability in update() as a separate updateType flag that is lit only when those select properties are changed on a node. It keeps all the logic for what determines the renderability in one easy to understand/debug place, and guarantees that the calculation only happens once even if multiple of those properties are changed at the same time on the node.

@wouterlucas wouterlucas force-pushed the feat/renderable-nodes branch from e810b76 to 4500a26 Compare December 14, 2023 12:05
@wouterlucas
Copy link
Contributor Author

What different properties do you anticipate other then color (and variants), texture (and image), shader and text?
And I don't mind changing it to be part of the update() loop but I suspect it would evaluated more often then just at get/sets of select properties. This is because you're going to run update() every frame, while the renderability wont change that frequent. Hence it being more expensive to evaluate in the update() loop then on set.

As it stands if the texture/shader/text are updated the renderability is not updated along with it. I'm simply proposing the recalculation of renderability in update() as a separate updateType flag that is lit only when those select properties are changed on a node. It keeps all the logic for what determines the renderability in one easy to understand/debug place, and guarantees that the calculation only happens once even if multiple of those properties are changed at the same time on the node.

texture and text definitely had renderability checks, however I did forget to hook it into loadShader

Added a new update:

  • Rebased with latest main
  • Hooked into update()
  • Added loadShader hook

Small note on updates vs renderability:
The update function is a result of changes that need to be processed, the renderability is a result of those updates, creating another update state for this felt counter intuitive. Hence keeping it stand alone. That also makes it easier to call into from texture or shader load/unloads that might be async.

Also the update() function is becoming quite large, chopping it up will make the JIT more likely to graph its callees and optimize those to the moon and back.

src/core/CoreNode.ts Outdated Show resolved Hide resolved
src/core/CoreNode.ts Outdated Show resolved Hide resolved
If the node has no renderable properties it will be skipped by the render step. Saving an otherwise expensive step in the render pipeline.

This will allow application / application frameworks to add nodes that do have X / Y / zIndex positioning for its children, but have no "renderable properties" that require to be drawn on the screen.

For example:
```
<ParentNode x=100 y=200 width=500 height=500 zIndex=2>
  <ChildNode x=10 y=10 src="..image">
  <ChildNode ...>
</ParentNode>
```

It allows you to freely manipulate the ParentsNode world position/width/etc. without it getting actually rendered. Prior to this change even ParentNode would get a GPU Render Pass, just have an empty instruction so the GPU gets the upload but then figures out it doesn't need to draw anything.

By forcing ParentNode to be color=0 from the App Framework like Solid or Blits, you can have the render still do the positioning but skip the rendering portion. For added clarity, that render step is 80% of the resources spent to render that node. Thus increasing overall FPS.

This helps a lot by avoiding render steps on nested nodes that do not require to be rendered, but are part of the tree for positioning / zIndex / alpha / container-ish logic.
@frank-weindel
Copy link
Collaborator

Testing this one along with my latest experimental changes. Should be ready to merge this soon.

@frank-weindel
Copy link
Collaborator

@wouterlucas I just noticed that one optimization that would be useful here is to also check if the width or height of the Node is 0. In that case it should be not renderable no matter what.

@wouterlucas
Copy link
Contributor Author

@frank-weindel agree! I’ll add that in about an hour or so.

@wouterlucas
Copy link
Contributor Author

@frank-weindel adding a check that checks for width and height per:

    if (!this.props.width || !this.props.height) {
      return (this.isRenderable = false);
    }

actually makes the alpha blending 2 test fail, the rocko figure isn't rendered:
image

Which is think would be correct, seeing its attributes in examples\tests\alpha-blending.ts:

    renderer
      .createNode({
        x: curX,
        y: curY,
        src: rocko,
        alpha: 1.0,
        parent: sideContainer,
      })
      .once('loaded', sizeToTexture);

However it does mean the behavior changes over previous and the test will need to be adjusted.

If you're okay with that I'll gladly push the change, otherwise let me know what would be a better route.

@frank-weindel
Copy link
Collaborator

I'll make a note to revisit it. I think its an important optimization but the texture/image should still load even if the Node is 0x0.

@frank-weindel frank-weindel added this pull request to the merge queue Dec 20, 2023
Merged via the queue into lightning-js:main with commit f9e82ea Dec 20, 2023
2 checks passed
@wouterlucas wouterlucas linked an issue Jan 16, 2024 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Performance enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mature/enhance update loop
3 participants