A "only-in-Rust" PR to open us off. Accessing multiple parts of the same data mutably is a headache because of the borrow checker.
Because we're not just using field access (this is a Mesh, with fancy/janky raw data access),
we have to reinvent the wheel ourself.
Rendering code is often very C-brained like this...
Reasonable code, nice tests.
It would be good to avoid allocations like beicause suggests, but that's non-blocking.
Does not warrant a release note though: this is cool and useful, but not at all interesting to casual Bevy users. Requesting changes to cut that before merging.
Docs! I love docs! Unfortunately we do have many different ways to spawn entities in a hierarchy, so it's nice to demo all these approaches.
I like the use of an enum + state-scoped entities to show all of the various approaches. Thanks Hukasu for reviving an old PR with your review!
I agree that bsn! would be really nice to demo, but this is an improvement as is,
and it's old enough that I want to just get it in rather than hassling the author.
Making a follow-up issue (nice and easy!) and merging.
Aaaaa. AAaaaa. AAAAAA!
Jesus Christ Malek what are you doing that wants this. Oh right, probably terrible cursed async nonsense.
Sure, fine, we can have this insane unsafe escape hatch.
Y'all better not ever use this.
Let me clean up the docs to distinguish the methods and make a follow-up issue for the NonNull thought mentioned in review...
Other than that, this is good to go. Shockingly straightforward. Merging with trepidation.
A dynamic resizing bug fix for rendering. Fixes flickering, sweet!
Reviewers are qualified and this is literally a one line diff. Let's pick up that improved doc change, then merge.
Ah state management. Bane of bugs.
Synchronize the computed camera information properly, by fixing our match statement.
Sure yes: not all NormalizedRenderTarget::None are equal, so it's cache-busting time.
Merging.
Perf time!
One of the classic, useful tricks is to change which algortihm you're using based on cheaply determined characteristics of the input.
Varying based on n (the size of the input) is particularly common.
The author did the same trick here, to avoid accidentally quadratic behavior, using an empirical THRESHOLD as the breakpoint for entity deduplication.
With benchmarks to boot, and a nice little graph showing that the curve is no longer curved :D
Lovely stuff.
I remember writing this code and realizing it would be O(n^2),
but I never expected anyone to want to simultaneously mutably access >40 entities at once!
I wonder if this is for scripting integration or something?
Anyways, complexity is low and the numbers are good, so let's merge this <3 Really excellent feedback from @Victoronz to help guide the author into this branched design; very clear and patient.
"When dragging widgets like sliders, the pointer often moves faster than the widget can follow. It drifts outside the bounding box while the button is held, breaking the interaction."
Perfect! This is exactly the sort of motivation I want to have written done in PR descriptions. Okay, that's a real problem: let's see if it's worth 562 LoC...
Okay, interesting, we're taking a "override the HoverMap" approach. That's interesting! Ah, much of this comes from the newly added example: nice.
Generally good, but two important omissions.
First, I agree with Talin that this should account for pointers getting cancelled / released.
Second, this needs to be listed in the HoverMap docs, to save some poor soul hours of time debugging.
Requesting changes.
I love corner cases. Rendering really is an endless exponential explosion of flags and state management...
Really nice to see a really well-reported bug get turned into an actual fix, from another user that ran into the same problem. And tested by (then approved by!) the original author! Yes, yes, join us: soon you will all be contributors.
This is a reasonable fix, and approved by one of our core rendering contributors (beicause). Merging <3
Mmm windowing bugs. I love windowing bugs. I've noticed this one: stuck keys after alt-tab etc.
This was a fairly gnarly PR to review and reason through, but I think that this is a sensible, defensive strategy to choose.
Let's merge this: this sort of problem is part of the endless long tail of stuff that makes programs (and frameworks) feel polished. Merge conflicts are easy; fixing them myself in the web UI.
Okay, interesting. A redo of a reverted PR, fixing a minor headache for rendering. Let's see what the story is.
Every fancy feature we add to our "standard" material is another "slot" (textures, sampler bindings)
that causes headaches for weaker devices (hi Android!) and takes up space that users might fill in with fancy custom properties using ExtendedMaterial.
Screen space transmission is one of those fancy features: tracing refraction and transmission through translucent materials using already computed color and depth information. Very nice, but definitely solidly in the "fancy" territory.
The previous attempt resulted in a nasty crash, and got reverted last minute so we could ship 0.19. "I can accept this since ScreenSpaceTransmission is a required component and enabled by default. We can add a way to disable it in a separate PR."
This is that PR. This seems like a simple, reasonable approach, and the reverting author approved this PR. Let's tweak the migration guide to improve searchability (this will change how transparent materials look!), and merge...
Solari go brr. Sure Jasmine, we can make this faster. "Lots and lots of painful code. This is kinda impossible to sanely review, but it works". Yeah lol, no kidding, +2890/-538 is brutal. The things we do for perf.
Nice to see the reviewers making an effort <3 Wait! Wait! This PR is already queued to merge! Caaarrt, you're stealing my content. Fine, I'll let this merge :p
Efforts to split bevy_math are finally blessed! Yay!!
I've wanted to do this for years but it needed a decision and was never really a priority.
Someone new (@JasmineLowen) took the initiative and prepared a split, which was easy to say yes to.
We love spinning out subcrates for self-contained functionality. Crate name is already reserved so it's time to Press That Button.
A PR I was pestered to review today! Bird (@Trashtalk217) ran a poll on Discord asking if anyone minded if we deprecated these, and the response was overhelmingly "??? Those exist?". I agree!
The fact that we can now replace these bespoke fancy structs using QueryBuilder is really exciting.
Great migration guide (with a theatricla Dutch accent but that's fine),
nice cleanup.
Merging :) Just wait until the follow-up PR: the diff will be properly negative.
Shinier large scenes! Really keen to have these around: Bevy's rendering is much better than it sometimes looks; the importance of well-made art in showing off the value of a scene is immeasurable.
This needs to be hosted properly though. "Random contributor's repo" is not the way. Let me look up where we stash them by convention... There we go. Review left with advice on how to do this properly.
We'll see what Github supports; Jasmine is right that these are crazy big. Really could use our own infra for this stuff soon, but that's far too much yak-shaving.
Back to the author it goes.
More splitting! More splitting!
Bevy has some truly arcane (world-class) curve machinery:
this should be its own crate, not lumped into bevy_math.
Easier to pull on its own,
and easier to leave out.
Joy of joys: what a wonderful day to be X-Blessed.
Merging.
Right, you need to determine whether to follow Mac vs Windows conventions for hotkeys at runtime when compiling for WASM.
Very sensible fix. I'm particularly glad for the comment in the Cargo.toml about the weird conditional dependency. That sort of thing is really annoying to understand when skimming, and this provides useful context.
Merging.
Mislabeled! This is actually S-Blocked.
I'm blocking (and other such efforts) this until we sort out the "feathers feature flag proliferation" problem. I don't want all of our examples to require specifying that flag to run. I also think we need to sort out some coding pattern things, as currently these ports are increasing code complexity each time we add them.
Indeed. Cargo's UX for specifying required features in examples sucks.
Relabelling and leaving alone for now. This is sad though: I love the little UI snippets!
Part of a very extensive effort to add proper High Dynamic Range (HDR) support to Bevy. Color is a massive, massive pain in the ass. It seems simple, but no! Endless nuance and complexity and bazillions of color spaces.
The core problem here is that we need provenance info for "what does this image mean by 'red'". If my red is not your red, we can't process everything out to get to the right place.
This has two approvals, but active pushback from a serious contributor. I'm sympathetic to their concerns around architecture here, but I do not have either the time or the rendering chops to make a final judgement call. Maybe if I had a week or two to study up, but HDR, while cool, is not worth prioritizing that hard right now.
Marked as X-Needs-SME: this can be the problem of our rendering SME's to deal with.
Another HDR PR! This one is more straightforward.
When composing colors together, you need to pick a single color space to blend them in. If you have multiple cameras stacked on top of each other, they all need to agree on that. Sure!
Lots of tests (thanks Claude...), but generally this looks like a reasonable approach to a real problem, solved via a fair bit of very straightforward code. I like to see that.
Merging; this is sufficiently polished, fixes current bugs and I do want HDR support. Man though, the LLMs are terrible at writing comments though: this was well within our policy and a ton of work was needed in review to remove the unclear cruft.
Oh my god we've been trying to square this circle for years.
Needing a seperate trait architecture only for systems that take &mut World has always felt
incredibly dumb and crufty, but no one has managed to figure out how to do this.
Apparently it... wasn't even that hard? Bizarre, I wonder when that changed. Our internals have undergone a bunch of mutation over the years to make things cleaner, so I suspect that at some point we accidentally unblocked this work.
Reading over the strategy, it's quite straightforward: just check this at runtime and expand the representable set of data accesses.
This even comes with a full migration guide, for all three of our users who are manually writing system impls.
"If you were using WorldId in an exclusive system..." uh-huh, yep, that's definitely me :p
Carefully done, and carefully reviewed. Nothing is jumping out to me as horrible, and the beneifts are really good.
ParamSet<(&mut World, OtherParams)>, as @chescock says??
Unification is here! Merge merge merge!! I would put this in the release notes if any end users actually cared.
Ooh, movement on asset processing. Good: this is shockingly important (if a bit dry / complex), and logn overdue.
Good explanation of why this is a good change in the PR description. Sensible little change, merging.
There's a fun story here: Rust's new trait solver is choking badly on this type due to the sins we committed. That's a bit of an upstream performance problem (and one of our contributors worked to improve that), but still, we're not literally a stress test for the Rust compiler. Shaving off 10 seconds on the critical path for Bevy compilation is no joke at all.
The code is mildly nicer, even if defining this does need a macro. Playing nice with the compiler dominates by far though. Even Bevy needs to stem its generics abuse sometimes :(
Merging. This is the breaking change version of this fix, and worth doing. We can / maybe should discuss a non-breaking version of this change backported to 0.19.2, but that's a problem for another day.
Holy cow, a tab deck? What did I miss while on vacation? Thank you jackdaw I love you jackdaw.
"This is the first PR in an effort to upstream the tab and docking functionality built for jackdaw, following the layered design from the tab panes and docking specification." Planning! Structure! Yay! This PR is big enough at +1073 LoC...
Dragging tabs, reodering tabs, styling, and dock layout are all planned for follow-ups
Good! That'll be fun to talk about in the 0.21 release notes. This sort of widget is wildly useful for things beyond the Official Editor: I'm really pleased with the composable, very public approach to UI we ended up with.
Release note needs an editing pass, but that's the expected case TBH. Functionality and design both seem solid: let's merge this <3
Panic due to underflow in a random Timer utility T_T
Boo!
saturating_sub is the clear right fix, and the line of doc comments is nice.
There could be a regression test, but eh, this code is never changing again.
Merging.
taffy! Our lovely adopted UI layout crate!
Thank you @nicoburns for maintaining this over the years; I really just don't have that "optimize layout algorithms" dog in me, you know?
Bug fixes and perf improvements? Sign me up! Flexbox is a goddamn nightmare of a spec: how are there this many bugs... Why is the only viable testing strategy "literally check what Chrome does in CI"??
Grumble-rambling aside, this looks great. An important version bump with a very easy (but not trivial) upgrade. Merging.
Mmm accessors for our private fields. Sure! This sort of thing is easy to forget when first making a new API: it's good that people are using this enough to complain and file easy PRs.
Merging.
Random build warning about dead code on Android. Yeet*!
*The actual fix is changing cfg flags, not deleting any code, but yeet is too much fun to say.
Docs that explain how to actually use something? That's not traditional :p
Very reasonable and extremely useful context. People shouldn't have to reverse engineer this from an example. Merging.
New UI layout algorithm supported? Better slap that in our visual snapshot testing to make sure it stays working.
Good discipline; this is important. Apparently Cart tried to merge this 5 days ago and it failed. Lemme kick the tires again.
This is part of why I really like the labels for "stuff that's ready to merge": it catches problems like that rather than losing the PR to the ether forever as it slips through notifications.
Support multiple full screen shaders at once. Sure!
Great answer from the first-time contributor explaining how they verified the fix :) Sensible reviews from trusted contributors.
There's a comment suggesting an example: eh, maybe useful for testing, but "multiple full screen materials" is very niche: not great pedagogically.
Not worth blocking on regardless: let's merge :)
Version bump for a random data structures crate we use in rendering. Notes promise perf improvements.
It's been 4 days and no one has screamed about supply chain attacks, so we should be safe ;) Merging; it's good to keep on top of these.
More tests? More tests! I didn't even need to open the PR to know who this was from.
"Testing: The tests should pass". Indeed, yes. No tests for our tests today.
Sure, these seem useful for verifying stuff during refactors. Merging.
Better panic handling, allowing us to handle nested catch_unwind calls.
And we're improving the error output while we're at it, sure :)
That's nice to do.
I had a question in review about "why on earth do we need a Option<SyncCell<Box<dyn Send + 'static>>> here,
which is now nicely recorded in a comment.
If I was squinting at it, so will future maintainers.
With that resolved, it's time to merge. Nice to see this sort fo hardening work: we're a real library now!
A stupid bug that highlights the value of our ornate Time infastructure.
Your settings should still save while the game is paused!!
As a result, this needs Time<Real>, not Time, which is implicitly Time<()>, which is inherently Time<Virtual>.
Even comes with a regression test.
Merging. Added to 0.19.2 as a backport-safe bug fix. Still not sure if we'll ship that (preparing releases is a hassle), but eh, it's there if we want to.
Little increment-decrement chevrons for numeric inputs!
I initially requested changes for this (use relations, show chevrons at all times), but was swayed by Talin: relations is a nice-to-have, chevron UX matches Blender's and is honestly pretty nice.
Sure, we can go with this. We can rework the UX of this later if others feel strongly after using it :) Merging.
It's still so cool to me that Bevy actually has widgets lol.
Today in corner cases: skybox + DLSS :(
Well, +1/-1 fix is perfectly nice. Merging.
#[must_use] is one of the lovely little ergonomic features of Rust.
It makes me happy, and I tend to go crazy with it in my own code :p
More static checks? Yes please! Bevy is more conservative, but given that this caught a real dead code problem in a different PR, this is 100% worth doing. Merging.
Better docs for "why on earth does Bevy have an ordered float wrapper". Great :)
The nitpicking about the IEEE 754-2008 standard and how FloatOrd relates is a bit silly to me,
but being more correct never hurt no one.
Merging.
@ickshonpe continues to work away, cleaning up niche layout problems in UI.
Did you know about FixedNode? What about OverrideClip??
Anyways, perfectly reasonable way to fix this: splitting the behavior out to avoid awful state management problems. Much nicer than running a hook or something.
Merging: the S-Needs-Goal was just mislabelling.
We do not in fact need formal process for tiny bug fixes!!
Code organization! We love splitting stuff out so it makes more sense. Except in ECS. ECS prefers giant 3k line files of the worst unsafe macro-laden trait soup you've ever seen T_T
We'll get there. This is a perfectly nice shuffle, and comes with a bonus test. Sure ickshonpe, I trust your judgement here: merging as trivial.