Update to new renderer #10

nicopap · 2021-12-22T11:15:00Z

This is a lot of code and hey, feel free to ignore it, I just wanted to hone my understanding of shader programming with the new bevy renderer. If you reject the PR, I really won't mind.

The code changes

The most notable changes in the code are:

There is no more Line struct. We store the colors and positions as buffer, pretty much the way it's going to be uploaded to GPU
We have two of those buffers, one for "immediate" lines (with duration=0.0) and one for "retained" lines (with a duration set), to be fair, I'm not even sure if it improves performance.
We use the attribute lists of meshes as buffers to upload to the GPU
We use the mesh vertex index to cull expired lines for the retained lines
We also maintain a list of index for the expired lines, we use it as a stack of available indices when adding new lines
The size limit now depends on the maximum number of vertex per meshes
The number of vertices is dynamic, so no massive pre-allocation, it evolves with usage

User-facing changes

Changes:

Need to use lines: DebugLines rather than lines: ResMut<DebugLines> to add lines
User Lines no longer exist
Depth check is now configured through the DebugLinesPlugin constructor

Regressions:

Does not work in 2d anymore
~~The depth check option is now always on~~

Improvements:

Does work with the new renderer
Slightly more portable, as it uses wgsl

Possible improvements and depth check

I wrote TODOs laying out potential improvement paths.

I tried implementing the depth test using an uniform. I started an attempt but when
I got to 100 additional lines of code I kinda gave up. Seems the best starting point
is the shader_material example in the bevy repository.

Edit: I now simply set it through the DebugLinesPlugin constructor.

Regressions: * Does not work in 2d anymore * The depth check option is now always on * Removed user Lines (not sure what their purpose was tbh) Improvements: * Does work with the new renerer * Uses wgsl I wrote TODOs laying out improvement paths. There are a few performance low-hanging fruits available. The depth check is kind of a monster. I wanted to have something out before trying it, because I'm strongly considering a uniform for this, so that the depth test can be modified at run time and is probably going to tank performance. Uploading uniforms to GPU under the new renderer requires an awful lot of additional code, due to the nature of wgpu, but it seems completely possible, just check the `shader_material` example in the bevy repository.

The persisting lines now are treated and stored separately from the temporary ones. This simplifies the logic for the temporary ones, and since it's the general case, it should improve performance.

nicopap · 2021-12-22T11:48:32Z

Some more notes: It should be possible to simply unset the indices attribute for the mesh used for "Immediate mod" lines, this way, we don't even have to fill_indices on them, just make sure mesh.indices == None. However, the new renderer doesn't yet support meshes without indices.

Toqozz · 2021-12-23T07:27:39Z

Wow! Thanks so much for the pull request!

All the notes here sound great and agreeable, but I probably won’t get a chance to properly review this until after xmas, since I’m away atm.

Will look properly when I get home!

# Objective Instead of panicking when the `indices` field of a mesh is `None`, actually manage it. This is just a question of keeping track of the vertex buffer size. ## Notes * Relying on this change to improve performance on [bevy_debug_lines using the new renderer](Toqozz/bevy_debug_lines#10) * I'm still new to rendering, my only expertise with wgpu is the learn-wgpu tutorial, likely I'm overlooking something.

With bevyengine/bevy#3415 it is now possible to upload meshes to GPU without `indices` set. Before, it would panic. Removing `indices` enables us to simply remove the `ImmLinesStorage::fill_indices` code, since the ordering is exactly as the GPU would interpret the mesh otherwise.

It is configured through the plugin constructor now. It makes more sense than through the `DebugLines` resource, since it's configured at the beginning and won't change in the middle of running the plugin. This was already true before, the difference is that it is now made explicit to the user.

README.md

Toqozz

I admit I haven't been following bevy 0.6 closely, but wow a lot has changed in rendering. I can't believe how complicated it is to do essentially the same thing we were doing before -- I thought it was supposed to get easier!

I really like what you've done to separate immediate and retained lines.

If the always_on_top depth test thing makes things too complicated, I don't mind dropping the feature until bevy makes it easier to do.

I'll have to look up on what's changed (and actually test this code), but why doesn't 2D work with this approach? This is a pretty big deal because 2D is probably at least 50% of this plugin's usage.

README.md

examples/3d.rs

examples/bench.rs

examples/depth_test.rs

src/lib.rs

nicopap · 2022-01-02T09:41:34Z

Regarding the other comments:

For 2d: I think it's because we do this in the Opaque3d phase. I briefly attempted to add the shader to the Transparent2d phase but failed. I didn't investigate much more because I was not interested in 2d support. Obviously if this is what people is the lib for, we need to add 2d support :P
For complexity: I actually have no idea. I'm a beginner in graphic code, and I'm unfamiliar with the way bevy does it. I have no clue what I'm doing. There might be a smoother approach. The bevy maintainers said however that the "low level" style of the API was intended, but that they would add later a more ergonomic API.

nicopap · 2022-01-02T09:57:39Z

I'll add the corrections you suggested and attempt to add back 2d support. I'll then set the PR as "open" and you can review again.

At the cost of making the `RetLineStorage::fill_indices` and `mark_expired` methods a bit more complex, it is possible to only store the line index rather than two point indices of expired lines. This should encapsulate neatly the complex underlying storage that mirrors the mesh attribute buffers. Other side benefit is that `timestamp` and `expired` vectors have their size divided by two. I also added the `Line<T>` struct to make the arguments to mutating methods of LineStorage more understandable.

nicopap · 2022-01-02T10:40:01Z

Regarding 2d support, this seems to be a blocker since I rely on mesh rendering for the lines shader.

Various name changes and extra doc string. Also fixes a likely issue with `mark_expired`, which would add duplicates to the `expired` stack.

Toqozz · 2022-01-03T01:36:17Z

Lol I accidentally marked this as ready for review and I don't know how to set it back, sorry.

Regarding 2d support, this seems to be a blocker since I rely on mesh rendering for the lines shader.

Ok. Guess we'll keep an eye on that.

For complexity: I actually have no idea. I'm a beginner in graphic code, and I'm unfamiliar with the way bevy does it. I have no clue what I'm doing. There might be a smoother approach. The bevy maintainers said however that the "low level" style of the API was intended, but that they would add later a more ergonomic API.

The main code is good, just the bevy buffer setup and stuff seems crazy, but yeah I think this is probably the API.

nicopap · 2022-01-08T11:16:50Z

With the 2d Mesh PR merged, I managed to add back support for 2d. But I had to separate 2d and 3d rendering based on a feature flag. I don't think it's possible to conciliate the two. Is it still OK for you?

This however, requires adding a feature flag :/

nicopap · 2022-01-09T08:03:39Z

For a summary of the changes:

Perfs are down :/ (I'm getting 120 fps on master while this implementation is getting 70 fps)
Additional feature flags makes the library harder to use :/
Way more code :/

I'm sorry. It's not the panacea. But it's somewhere to start.

nicopap · 2022-01-09T12:40:10Z

After doing some tracing, using the bevy/trace_tracing feature. I found most of the time is spent adding the lines to the LineStorage in bench.rs. There is also a non-insignificant amount of time spent copying the meshes from the app world to the render world, but it's not that much compared to the time spent in the demo_circle system.

I have difficulties interpreting the results of tracing. Timing precisely the line insertion operation, it seems add_line is of the nanoseconds order of magnitude, while line_gradient is in the microseconds range.

After experimenting a bit; for example, reversing the Line<Vec3> and Line<Color> function arguments (maybe it's the struct construction and deconstruction adding unneeded operations?), and directly accessing ResMut<ImmediateLineStorage> from bench.rs (maybe it's the indirection and the calls to Time::seconds_since_startup that makes things slow), I fail to see any improvements.

I really don't see where the performance degradation comes from. Looking at the current code, there is really not that much more going on when inserting lines. The one sure source of slowness is the 2.4% higher MAX_LINES (from 128000 to 131072) but it doesn't explain why it's about 2 times slower.

This inspired me though. I think I can make DebugLines a Res again.

nicopap · 2022-01-11T06:50:37Z

@Toqozz I'm done with this. I've no further improvements to make, unless you have any thing to point out.

Toqozz · 2022-01-11T07:05:00Z

Hey, sorry but I won't be able to take a proper look at this until Saturday.

Honestly though I'm happy to merge these changes into master for now (we don't guarantee stability on master anyway), and I'll come back to look at it properly and release the new version.

Toqozz · 2022-01-11T07:05:13Z

Thanks for all the work you've put in! <3

CleanCut · 2022-01-11T21:20:20Z

This mostly works for me. Two problems:

The line won't stay in front, even though I'm doing .add_plugin(DebugLinesPlugin::always_in_front())
I think the line is even thinner than before. I can live without this being fixed, but it would sure be nicer if it were thicker.

nicopap added 2 commits December 22, 2021 08:33

Separate retained and immediate lines

95ee329

The persisting lines now are treated and stored separately from the temporary ones. This simplifies the logic for the temporary ones, and since it's the general case, it should improve performance.

This was referenced Dec 22, 2021

[Merged by Bors] - Implement non-indexed mesh rendering bevyengine/bevy#3415

Closed

chore(deps)!: require bevy 0.6 jcornaz/heron#161

Merged

nicopap added 3 commits December 23, 2021 21:25

follow bevy main HEAD

eae89a4

Toqozz reviewed Dec 29, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Toqozz requested changes Dec 29, 2021

View reviewed changes

Toqozz mentioned this pull request Dec 29, 2021

Make lines drawn the same frame they are called #11

Merged

nicopap marked this pull request as draft January 2, 2022 09:56

nicopap added 2 commits January 2, 2022 11:44

Remove references to thickness in examples

7bf00c8

Refactor and document

0d99d61

Various name changes and extra doc string. Also fixes a likely issue with `mark_expired`, which would add duplicates to the `expired` stack.

nicopap force-pushed the bevy-main branch from 1040ed5 to 0d99d61 Compare January 2, 2022 12:03

Toqozz marked this pull request as ready for review January 3, 2022 01:32

Add back 2d renderning

5c8e843

This however, requires adding a feature flag :/

nicopap requested a review from Toqozz January 8, 2022 11:47

Update bevy dependency to 0.6

1202f2f

Revert to exposing DebugLines as a bevy Resource

efe4d3c

nicopap added 2 commits January 9, 2022 14:08

Fix minor spelling errors

134c579

Do not bump already-bumped version

9355704

nicopap marked this pull request as draft January 11, 2022 06:50

nicopap marked this pull request as ready for review January 11, 2022 06:51

Toqozz merged commit bc49414 into Toqozz:bevy-main Jan 11, 2022

Toqozz mentioned this pull request Jan 11, 2022

Bevy 0.6 #13

Closed

Toqozz mentioned this pull request Jan 17, 2022

Current main branch introduces stutter #14

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to new renderer #10

Update to new renderer #10

nicopap commented Dec 22, 2021 •

edited

Loading

nicopap commented Dec 22, 2021

Toqozz commented Dec 23, 2021

Toqozz left a comment

nicopap commented Jan 2, 2022

nicopap commented Jan 2, 2022

nicopap commented Jan 2, 2022

Toqozz commented Jan 3, 2022

nicopap commented Jan 8, 2022 •

edited

Loading

nicopap commented Jan 9, 2022

nicopap commented Jan 9, 2022

nicopap commented Jan 11, 2022

Toqozz commented Jan 11, 2022

Toqozz commented Jan 11, 2022

CleanCut commented Jan 11, 2022

Update to new renderer #10

Update to new renderer #10

Conversation

nicopap commented Dec 22, 2021 • edited Loading

The code changes

User-facing changes

Possible improvements and depth check

nicopap commented Dec 22, 2021

Toqozz commented Dec 23, 2021

Toqozz left a comment

Choose a reason for hiding this comment

nicopap commented Jan 2, 2022

nicopap commented Jan 2, 2022

nicopap commented Jan 2, 2022

Toqozz commented Jan 3, 2022

nicopap commented Jan 8, 2022 • edited Loading

nicopap commented Jan 9, 2022

nicopap commented Jan 9, 2022

nicopap commented Jan 11, 2022

Toqozz commented Jan 11, 2022

Toqozz commented Jan 11, 2022

CleanCut commented Jan 11, 2022

nicopap commented Dec 22, 2021 •

edited

Loading

nicopap commented Jan 8, 2022 •

edited

Loading