0.15 Retrospective #16431
Replies: 8 comments 43 replies
-
|
Writing release notes / migration guides is currently a huge slog. Especially for @alice-i-cecile and @cart since they have to write the release notes for the feature authors that don't do it themselves (which is quite a lot). Maybe it's an idea to include the writing of release notes before merging PRs? Make it as standard as writing documentation and tests? |
Beta Was this translation helpful? Give feedback.
-
|
I would much rather write the docs/migration-guide/release-notes while it's fresh in my mind. I also know I absolutely won't manage to do that consistently unless I'm required to. The current level of friction (having to open a second PR in a second repo) doesn't help. |
Beta Was this translation helpful? Give feedback.
-
|
I also like the idea of frontloading this work. Currently I'm liking the idea of putting release note markdown somewhere in the Bevy repo and requiring it as part of the review process. The trick is that we can't / shouldn't enforce this everywhere, so how do we build the process in such a way that (1) we don't encumber non-feature changes / changes that don't need release notes and (2) we don't accidentally drop features by not forcing them through the process. |
Beta Was this translation helpful? Give feedback.
-
|
Perhaps the |
Beta Was this translation helpful? Give feedback.
-
|
There's a CI action that checks that PRs with the label In the same way, we could check on a label if the release note markdown file is present on label I think it's OK to require the file to be present, but a PR shouldn't be blocked on its content being final |
Beta Was this translation helpful? Give feedback.
-
Agreed. I think a first-draft is sufficient to improve the status quo massively. Suggested changes and follow-up PRs can refine these documents after the fact anyway. |
Beta Was this translation helpful? Give feedback.
-
|
It's really hard to exhaustively / automatically test rendering features across platforms. Are there any CI solutions that could be implemented. Require more extensive tests from feature authors? |
Beta Was this translation helpful? Give feedback.
-
|
Pixel Eagle by @mockersf is the start of a cross-platform screenshot based tool to better test rendering. I'd also like to build out better tools to test rendering features within a single binary: recompiling for each feature sucks. The Simply asking for more tests from feature authors won't work: we need a real framework to write good rendering test code in rather than just saying "figure it out" on an ad hoc basis. Relatedly, folks are itching to drop webgl2 support, which will help with random breakage (it's very limited), but won't fully resolve things. Rendering will continue to be hard to test (manually and automatically) and prone to platform-specific breakage, and we need to design solutions with that in mind. |
Beta Was this translation helpful? Give feedback.
-
|
I do wonder if it'd be possible to support some kind of headless rendering tests. Even if it can't check driver support, it should be able to at least write to a texture and verify it's output, to make sure that at least rendering isn't catastrophically broken. |
Beta Was this translation helpful? Give feedback.
-
|
The issue in tests isn't headless, it's easy enough to simulate a window (just works on macOS and Windows, and a virtual frame buffer on Linux) or indeed just run headless. The difficulty is having either a GPU usable in tests, or CPU rendering.
Our tests in CI already run a few rendering tests, save the result to a screenshot and check it didn't change. Coverage is hard to extend because:
A solution for those difficulties could be to throw money at it, either by hosting dedicated runners somewhere with the hardware we need or by paying for larger runners on GitHub (it should work, but I haven't actually tested that) The current tests executed are:
The last two points currently are still checked manually because they were added recently and I wanted to check their reliability before adding reporting to them. They've been working well for a few weeks, so it's probably time to take them to the next step, which I'll document soon in #15918 |
Beta Was this translation helpful? Give feedback.
-
|
There's one major blocker for me to writing (unit/feature) tests, as opposed to end-to-end ones, and this is winit and the inability to use the default test harness due to tests not running in the main thread. This prevents testing small pieces of GPU code which run a lot faster (few seconds) than a full featured end-to-end test, even on software renderers. This has been and continue to be a major pain point on Hanabi, and I have to rely to visual testing of examples, which is orders of magnitude slower and more tiring. I do have limited GPU tests under a feature flag, which test things not tied to the render world. But that's all I can do for now. And that leaves a big gap in coverage (I can't get past the 50% coverage point, and that's not due to lack of time). One word on visual testing via screenshots / image comparison: from (professional) experience, the time investment to setup and maintain is not worth the marginal coverage increase compared to targeted manual testing on each change, because you're testing Bevy as a whole when you do that, and any breakage in other parts can break the end result on screen without the render code actually being the issue. Writing actual GPU tests (write data to GPU, execute GPU commands on data, download resulting texture or buffer content back to CPU, and validate data) provides a lot more bang for bucks. |
Beta Was this translation helpful? Give feedback.
-
|
The retained rendering PR turned out to be a bit of a problem. Merged near the end of the cycle, it caused quite a lot of bugs. These bugs had to be ironed out on main which is extremely frustrating for everyone involved. Since I was pretty heavily involved, I'll give my perspective first, including what I could've done better / how the process could be improved. The Initial ProblemIn 0.14, at the end of every frame, the render world purged all its entities. For complicated ECS reasons which I'm not going to explain (you can get a jist here), this blocks relations. Because I (and a lot of other ecs folks) care about that, I set out to try and fix it. The ProcessI made my first attempt at July 9. The exact way I did it was not really important, because I barely got anywhere before I managed to nerdsnipe @re0312, who created this PR on July 23. I want to take one sentence to say that @re0312 is a much better coder than I am and if it weren't for them I likely wouldn't have gotten anywhere. This PR went through the review wringer, and while I quite often asked around in the Discord for reviews I was often told that people were busy (which is completely fair, large parts of this were during the summer vacation, so I understand that people had better things to do). People are busy, that's just how it is. The PR became quiet after a bit, so on September 20 I decided to push it the final stretch. I figured it'd be a shame if all that work went stale. And I should add that this PR grew stale very quickly because it touches almost every single part of the rendering machinery. In any case, I tried to hurry the merging of this PR, in large part because I was done with it. This included a tactic where I tried to only get a few important reviews from SMEs / Alice, because more input would again slow the PR down. When it eventually did merge, I figured that there were probably still bugs in the code. Places I or re0132 forgot, although I hoped that everything would go off without a hitch. Or at least, I hoped that the bugs wouldn't be that bad. Hope is not a great code review strategy. In any case, I wrote up a follow-up issue and largely pulled my hands from it, because I was very busy and my attention was needed elsewhere. What Went WrongThere turned out to be a bunch of bugs. Some of them I had foreseen (entity leaks, wonky extraction), some of them I had theorized but deemed unlikely (removing only a synced component from an entity instead of removing the entity leaves said entity behind in the render world). By and large I don't think there was much that was technically insurmountable given the design, but it sure was annoying to track down every single bug. I'm very sorry to and thankfull for the people that did. On a personal level, I got inpatient, and tried to force through a PR that clearly wasn't ready. On a slightly deeper level, I didn't know how I could ever test the fullness of the change without just pushing it to main and having a thousand eyes create issues. This PR clearly needed a strategy for transitioning to a retained render world, but I don't know how that would've looked like. Please let me know, if you do. I think that this PR has kind of flew under the radar (in the end that was kind of intentional). This change is important for the ECS folks, but largely involves rendering code. They Rendering people seemed less interested in this refactor. The fact that I took this on without having every played with the rendering internals before, should probably be noted somewhere. Also, because this was a refactor, I think a lot of people who looked at it, underestimated how much could break, right untill everything broke. I'm out of thoughts for now, if anyone has questions, let me know. |
Beta Was this translation helpful? Give feedback.
-
|
"blessed" is supposed to mean the controversial nature of something has been resolved, not that it's on a fast track to being merged. it should still follow the nomal process to be merged, with enough reviews by people with enough expertises we could have had three source of controversy on that change:
1 was blessed, I haven't followed the discussion enough to know about 2, and 3 could have been resolved with more reviews without going through being controversial |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, there was agreement in principle from both ECS and rendering leadership, but the changes and migration path and implications were not adequately reviewed and discussed. |
Beta Was this translation helpful? Give feedback.
-
I do understand these concerns, and acknowledge that "efforts dying out via leadership neglect" has been and continues to be a problem to solve (although I hope we can all agree that things have gotten better recently, which is a result of "letting things happen" that we would not have in the past). We were very lax last cycle when it came to reviews. In many ways it was good as it let people "just get things done" / let the project grow. But this is me sounding the alarm bells on something that I strongly believe went too far. If we allow architectural choices in this category to be made in this way, the quality and vision of the project will de-cohere in a way that makes me deeply uncomfortable. I would rather we do not move forward at all (for core architecture things), and yes, risk burning contributors out, than move forward in this way. If the SMEs are not steering the ship, it should not move. If the ship isn't moving fast enough, we either need to strategically and intentionally stop while SMEs are off doing other things, or we need to find more SMEs. |
Beta Was this translation helpful? Give feedback.
-
|
@NthTensor I do also want to call out that I appreciate that people have pushed back against me being "too precious" with the code in the past (and this includes you!). I was standing in the way of progress. And I'm glad there are still people applying counter forces and challenging my assumptions. But I still strongly believe I am right about cases like this, and I intend to hold this line going forward. Bevy is good in a large part because we are careful and thoughtful about architecture, and because we have held lines like this in the past. |
Beta Was this translation helpful? Give feedback.
-
|
Absolutely! We have drastically improved the contribution process over the last year, and I am deeply appreciative of the choices that project leadership made to achieve that increase in velocity. We clearly benefit from your oversight, and the tools at your disposal to prevent decoherence and destabilization must be kept in good working order. Stability/velocity is a delicate balance, but it's clear from these responses that our position on that scale is something the leadership treats with great care and consideration. So long as that's true where I would draw the line in this particular case is really beside the point, and I for one am satisfied. |
Beta Was this translation helpful? Give feedback.
-
|
I'm really pleased about the success of working groups. Continuing the trend started with bevy_color, the text rework, bevy_picking, curves and the next generation scene working groups have all been really effective at delivering high quality work in a cohesive and collaborative way. Having explicit blessing, a design, and a place to talk really helped! |
Beta Was this translation helpful? Give feedback.
-
|
I agree with this! I've had a great experience joining working groups and helping work on larger goals. Working groups are great to find contributors with similar interests and collaborate with them on issues in a semi-official manner. |
Beta Was this translation helpful? Give feedback.
-
|
Is there currently a way to know which topics are considered for a working group? It's easy to discover the ones which have already been formed, but not sure how to find potential future ones. |
Beta Was this translation helpful? Give feedback.
-
|
I kinda don't think we should have a list? Much prefer that they come about organically because people have been thinking about/struggling with something in the engine. Anyone can propose a group, and any proposed group will get consideration for scope & alignment by the leadership team. Agree btw, I continue to be really happy with the wg process. I think we are still struggling to get people to write down & document their designs, but maybe that will come with changes to release notes. |
Beta Was this translation helpful? Give feedback.
-
|
I want to give a little bit of pushback with regards to the relations working group. As far as I can tell we do have a clear goal before us (both an rfc and a hackmd document), but the work has been somewhat slow. I mostly blame this on a lack of manpower. James has been busy at work, same goes for me and periwink. ECS threads also have a rather unique ability to derail into hypotheticals like: 'could you build a relational database with relations?' or 'How should we handle thing that flecs hasn't even implemted yet and is still at least a year away?' I understand that I can't stop people from talking about things they're passionate about, but it does kind of annoy me sometimes. Still, the relations working group is no worse than the ecs-dev channel, so it's a wash on that point. I'm kind of wondering if the 'self-contained task' this working group is doing is too large, but I'm not sure. From where I'm standing, landing an MVP for relations is possible in 0.16, but I said the same about 0.15, so I might just be delusional. |
Beta Was this translation helpful? Give feedback.
-
|
On the outside, the working group threads were helpful as an early adopter. I had a place I could ask questions related to an implementation detail or bug or give feedback on the feature, etc. |
Beta Was this translation helpful? Give feedback.
-
|
Upstreaming bevy_picking went really well overall, and I think it's a model to emulate. This is a really effective way to rapidly expand our capabilities, and it helps share the maintenance burden for vital functionality. I think in the future we should consider polishing / improving further in the upstream repo (docs and examples especially), and I underestimated the difficulty of the work: it ended up taking fairly experienced engineers to do the integration work effectively. |
Beta Was this translation helpful? Give feedback.
-
|
@NthTensor I'd be interested in your thoughts on this briefly, as you ended up largely leading this effort. @Jondolf and @aevyrie as well, since you were also major contributors / the upstreamed crate's author. |
Beta Was this translation helpful? Give feedback.
-
|
I thought it went really well. I was away for personal reasons for the majority of the process, but it went smoothly from my perspective. I get the sense this area of bevy could use a SME; there is a lot of surprising complexity, and even after 0.15 there is still a good amount before we have "finished". It's a bit difficult to feel like the decisions you make and the design you are settling on is going to stick without a dedicated SME present. My biggest worry is that without someone with depth of knowledge, context, and a clear vision, this code will begin to rot from well-meaning but poorly thought out changes. |
Beta Was this translation helpful? Give feedback.
-
|
Agree with @aevyrie here about an SME, although I think Alice serves this roll pretty well. I'd like to see us upstream more stuff. It's a challenge, but a different kind of challenge than greenfield work, which mostly comes from solving problems in the ecosystem crate by modifying the ecs or the other core crates (the for bevy_picking this was adding bubbling to observers). IMO it's something you're usually going to need engine-devs for (which was the issue with the student group, not their fault). For future up streams, this is probably how I'd work it:
I probably wouldn't try to plan more than one of these stages at a time. It's better to bite off manageable chunks. |
Beta Was this translation helpful? Give feedback.
-
|
The new workflow for writing migration guides / PRs with each piece of content in its own file is great (other than being done later + in a different repo). Almost no merge conflicts, super easy to re-organize, and the new PR merging strategy is fantastic. Really pleased with how that design and tooling work has paid off. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed! Biggest downside is it breaks hot-reloading so you can't preview your changes inline (instead you have to stop and start |
Beta Was this translation helpful? Give feedback.
-
|
At some point in the 0.14 cycle I hacked around the lack of hot reloading with this masterpiece, dug up from my discord history: fswatch -0 -o release-content | xargs -0 -n1 -I{} sh -c 'touch content/test.md && rm content/test.md'But I completely forgot about that for 0.15. Also from that chat:
But configuring additional paths to watch feels like something that they may be okay with. (just guessing) It's also a huge pain to have to dig up the corresponding file for each section that needs editing while reading through the rendered document. When sections need to be combined, this gets even more tedious / error-prone. I recall starting to read through the draft a while back, realizing how many sections needed rework like this and just giving up. |
Beta Was this translation helpful? Give feedback.
-
|
Just remembered this and threw out a feature request here: getzola/zola#2741 |
Beta Was this translation helpful? Give feedback.
-
|
Good call. Just added some more context |
Beta Was this translation helpful? Give feedback.
-
|
As usual I am very happy with the response time and quality for PRs, reviews and dev discord questions, it makes the process fun and gives me confidence to keep contributing 🚀 |
Beta Was this translation helpful? Give feedback.
-
|
Some breaking change #16163 went in after the last Release Candidate ( |
Beta Was this translation helpful? Give feedback.
-
|
Yeah, I'd love to have a section on features in the contributing guide TBH. |
Beta Was this translation helpful? Give feedback.
-
|
Agreed regarding feature management. 0.16 is going to include even more features due to the A particular pet peeve of mine has been feature discovery. The only reliable way to see what features a crate has is to just read the |
Beta Was this translation helpful? Give feedback.
-
|
Feature discovery / documentation is a rough spot in Rust in general; let me ask around to see if there's anything in the works there. |
Beta Was this translation helpful? Give feedback.
-
|
There is a flags page on docs.rs, which is what I usually reference. |
Beta Was this translation helpful? Give feedback.
-
|
For Bevy we build this page: https://docs.rs/bevy/latest/bevy/#cargo-features
There's the https://crates.io/crates/document-features crate/macro that enable more fancy documentation, but doesn't enforce that features are documented on documentation, the pain points (for me) are:
|
Beta Was this translation helpful? Give feedback.
-
The release cycle is quickly nearing it's end. During this cycle, some things went well, while others went poorly. Leave your feedback here, so we can hopefully improve the process for upcoming release cycles!
Beta Was this translation helpful? Give feedback.
All reactions