Make post_process_write() take a mutable reference, fixing ordering footgun

[coreh]

While experimenting with a custom lens flare post-processing effect, I hit erratic behavior related to system order, that turned out to be a previously reported interior mutability footgun (#24839): post_process_write() doesn't require a mutable ref, so it allows multiple systems to concurrently hold write access to the same view target, unless you carefully specify ordering.

Previously this wasn't really an issue because of the render graph, but #22144 made it easy to hit by accident if you forget to add the relevant .before()/.after(). (And you get no ambiguity warnings because the systems don't share mutable access)

Objective

Fixes #24839. An alternative approach to #24961 (but we probably still want to make the system order explicit in the example)

Solution

• Make post_process_write() mutable
• Update all relevant queries to be &mut
• Resolve now reported system order ambiguities:
  • Moved msaa_writeback and various shadow passes to Prepass
  • Added an AntiAliasingSystems set, as proposed here by @IceSentry, flagged systems in the set as ambiguous with each other

Testing

• Did you test these changes? If so, how? Yes, visually compared the output of various rendering examples against main to avoid regressions, and verified the HDR post processing issue is no more.
• Are there any parts that need more testing? DLSS (since I don't have nvidia hardware to test this on)
• How can other people (reviewers) test your changes? Is there anything specific they need to know? Verify the ordering is correct and behavior is still consistent
• If relevant, what platforms did you test these changes on, and are there any important ones you can't test? Tested on macOS, on an M3 Pro. Ideally need PC hardware/verification on other platforms

[JMS55]

Does this kill parallelism?

[coreh]

Mechanical edit, haven't been able to verify this yet since I don't have an NVidia card easily avaibale for testing

[coreh]

Does this kill parallelism?

I'm not entirely sure. I think most of the things this affects were already “supposed” to run sequentially to produce correct results anyway, or were coincidentally/transitively made sequential, so if they were underspecified, the performance/parallelism gain was already due to a bug.

The various shadow pass systems all take &World though, and as I moved a few things around to Prepass, this might have changed the perf characteristics a bit.

What is the ideal way to benchmark/visualize this?

[JMS55]

Not sure what scene I would suggest (maybe beyv_city? something complex enough to have significant command encoding costs), but use trace_tracy to view parallelism.

Also, great to see you contributing again!

[beicause]

Disagree with the solution. It's odd to make methods that don't need mutable reference become mutable, as it may lead to unnecessary restrictions, e.g. order independent post processes should be able to run in parallel.

[coreh]

Hmm... I agree the &mut without actually using it for anything internally is a code smell. Maybe there's a case to making it actually need the mutable reference, dropping the atomic

My hunch is that two systems having concurrent write ability to this is pretty much always unsound

Edit: oops, accidentally the wrong button