Oh, this is an interesting PR. It's a nice little vendored + adapted data structure used for rendering thing.
Code itself is fine and this is reasonable to want. Nice benchmarks! Three questions though!
First, is this something we should be vendoring? Yes, we have a meaningful extension / variation: you can set the alignment at runtime. Great, go crazy shader sickos.
Second, is bevy_platform the right spot for this?
This was, at least originally, a polyfill crate to provide std-flavored functionality across
all of the no_std but yes_alloc platforms that we support.
As a result, it collected a lot of data structures.
Which makes sense for why the author would put it here.
Eh... that's fine. This is as good a home as any: better here than bevy_utils I guess.
Finally, licensing.
Can we legally use this vendored code?
Good: module docs credit rkyv explicitly.
Crate is MIT: perfect.
That's probably legally sufficient (there's attribution, and a copy of the MIT License already in the repo), but we can do better. Affixing a note about the author's name + copyright year + "used under MIT" directly. That's simply more polite and clear!
With that done, I'm happy with this. Rendering seems excited about the possibilities here, so let's merge.
"Use the ECS to store the render world data needed for windows. A lot of this code was written before we had a retained render world which explains why it never stored the data on an entity"
Thanks IceSentry, I couldn't have said it better myself. Yay useful PR templates!
It's good to get this tech debt addressed, and this is pretty straightforward. I suspect that this was something he noticed when poking at alternate windowing backends for Bevy. Always nice to have options.
Code is good, and this is uncontroversially the right pattern here. Merging. Wait, merge conflicts. Amended: pestering the author on Discord about merge conflicts!
Ooh, it's rendering time, which means decoding acronyms! I'm getting better: I know most of these.
DI = Direct Illumination: the contribution towards lighting of stuff shining directly. GI = Global Illumnination: lighting caused by stuff bouncing off the environment in complex ambient ways. MIS = Multiple Importance Sampling: a fancy statistical sampling approach that weights the hard cases more. VRAM = Video Random Accesss Memory: RAM / memory, but on your GPU.
Apparently TLAS = Top Level Acceleration Structure: the thing that holds your data to make spatialized look-ups fast.
Okay, after a few minutes of pondering the strategy makese sense to me: if you use more up to date data you'll get less lag, which means better looking results for moving shadows.
Those are the reviewers I want to see here, and there are no terrible sins hiding in the code. Cool to see @JMS55 taking ideas from a new rendering contributor for Solari! Merging.
Ooh I helped with this one. Turns out, some applications of data structures want different performance characteristics even if they're storing the same kind of data. Shock and horror.
We moved from a HashMap backing to an IndexMap backing for a random helper data structure,
fixing bugs and improving things for some users, but regressing perf for others.
This PR partly undoes that, by splitting apart the types so callers can choose which one they care about.
The interesting bit is the migration story.
The original author tackled this by simply quietly changing the behavior of TypeIdMap,
and creating a TypeIdIndexMap for the use cases that cared about ordering.
There was a migration guide, but that's easy to miss!
Instead, I suggested two symettric type names, and a deprecated type alias that preserves old behavior. Much easier! Forces all callers to think about which set of tradeoffs they want.
Anyways, with that fixed up this is really quite a simple PR.
The names are longer but clearer, and frankly, when was the last time you typed TypeIdMap.
Merging :)
"Works on my machine" would be such a nice, easy thing to do. It would make so many of my headaches maintaining Bevy go away. Usually that's "graphics drivers" and "windowing bugs", but today it's "non-Latin keyboard layouts".
We should do it right though. Copy-Paste etc functionality should just work out of the box for anyone in the world using Bevy apps, and we need to set an example of how to do it right.
Careful fallbacks, cribbed from web. Yay thank you first-time contributor: real "diversity is our strength" moment right here.
Merging, but once BEI is upstreamed I am agressively moving this into the input-manager functionality rather than random UI code, with docs everywhere pointing people at the right way to do it.
UI bug fixes! You know that means:
interactive testing, the good-old-fashioned way.
A little gh pr checkout 25057,
a little cargo run --example feathers_gallery --features bevy_feathers and...
Compiling. Right. One second. Tested the fixes, reviewed the code, all looks well in the world.
Good! Merging. Sure is cool to have a real GUI framework after all this time.
Did you know that Bevy ships little helper macros to assist you with control flow?
Don't want to continue if there's an error! bail!
You can, as they say, just walk away.
Now, we're adding ensure!, which works on boolean values.
Neat: very readable, very terse.
This sort of thing is a good use of macros :p
Shamelesssly cribbed the idea from anyhow: thanks for the inspiration!
Anyways, merging; enjoy.
Rendering performance work! +16/-36, with tracy profiles. Neat!
Ugh, what am I even supposed to find? Rendering folks must have been raised on those spot the difference games...
It's 0.4 ms faster? That's a start. There's better parallelism (seen by overlap) in the second tracing graph? IDK man. Some explanatory text would have been good.
Still, I'd merge this PR even without the visual aid. The explanation makes sense, the numbers are better, and we have rendering expert sign-off.
Merging.
A little helper for the text stress test, making it easier to test the more normal case where the text is not wildly animated.
Sure, merging: that makes a lot of sense.
It's still a bit weird to me that our stress tests use command line flags. I think maybe it makes them easier to automate than a little widget? IDK, I suspect that's a just-so story: they predate our widgets, and maybe even our janky on-screen "press F to pay respects" instructions. Neutral theory is hard to beat!
Anyways, let me know if you have opinions on this!