bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.67k stars 3.61k forks source link

Bevy Development Process #2256

Closed NathanSWard closed 2 years ago

NathanSWard commented 3 years ago

Rendered

What problem does this solve or what need does it fill?

A clearer defined set of rules and practices of project management issues for the bevy development process.

Since bevy is exponentially growing and more and more contributors are entering, we need a better defined practice for managing and organizing issues and pull requests.

What solution would you like?

Introduction of a proper bevy project board inside github, with automated movement between columns based on reviews/approvals etc. Revamp of github labels. A triage-team or something of the sort for those in change of assigning proper labels.

What alternative(s) have you considered?

Relying on the current tag/labeling scheme.

Using external kanban board software to track issues.

A few helpful resources

Github project board automation

TODO

NathanSWard commented 3 years ago

I'm still a fan of adding the appropriate type emoji on a PR title, if this is something we want to pursue. It makes it simple when viewing commits (as the title becomes the first commit line) and being able to tell at a glance what a commit did without reading into it. ☺️☺️

At least I would want to add them to my PRs.....

alice-i-cecile commented 3 years ago

Can you test what it does to relevant links in a test repo @NathanSWard? That's my largest hesitation left on the gitmoji front.

NathanSWard commented 3 years ago

Can you test what it does to relevant links in a test repo @NathanSWard? That's my largest hesitation left on the gitmoji front.

By relevant links, do you mean when they live in the labels? Since my current proposal would be to remove them from labels, but add them to the PR titles.

alice-i-cecile commented 3 years ago

Oh wait, the PR title isn't part of the PR links. Okay, we're good then.

mockersf commented 3 years ago

On the lifecycles, there are no transition to reject / close issues or PRs.

Labels should be prefixed by something indicating their kind: T-Enhancement, C-Audio, P-Web, .... This helps identify them when reading, and also help when adding/editing a pr/issue labels as they will be sorted.

About reviews:

Once in the Reviewer Approved column, the PR will be officially reviewed by Cart.

I think it's more "the PR will me marked as S-ready-for-official-review(or S-ready-for-cart while Cart is the only final reviewer, but I would love to move away from naming people in labels). Cart reviews are (currently at least) not direct.

About emojis in PR title: while I like having emojis in the git log, it feels strange having it both in the label and in the PR title. If they differ, what happens?

NathanSWard commented 3 years ago

On the lifecycles, there are no transition to reject / close issues or PRs.

Hm yes this is true. There initially was however since we use bors, the original PR is marked as closed so there is no way to distinguish between a PR closed by bors and a PR closed because of another reason. A possible solution to this is adding an closed status label which we can put on these PRs?

Labels should be prefixed by something indicating their kind: T-Enhancement, C-Audio, P-Web, .... This helps identify them when reading, and also help when adding/editing a pr/issue labels as they will be sorted.

Yep I like this a lot!

I think it's more "the PR will me marked as S-ready-for-official-review(or S-ready-for-cart while Cart is the only final reviewer, but I would love to move away from naming people in labels). Cart reviews are (currently at least) not direct.

Agreed! I think having a specific label for ready-for-official-review would be greatly beneficial and would help organize the PRs.

About emojis in PR title: while I like having emojis in the git log, it feels strange having it both in the label and in the PR title. If they differ, what happens?

So my solution would be to only have the emojis in the PR title and not in the label attached to it.

mockersf commented 3 years ago

On the lifecycles, there are no transition to reject / close issues or PRs.

Hm yes this is true. There initially was however since we use bors, the original PR is marked as closed so there is no way to distinguish between a PR closed by bors and a PR closed because of another reason. A possible solution to this is adding an closed status label which we can put on these PRs?

PRs merged by bors are prefixed with [Merged by Bors], can you catch that?

NathanSWard commented 3 years ago

PRs merged by bors are prefixed with [Merged by Bors], can you catch that?

Not with github automation afaik :(. However, I just updated the PR labels to include a S-Closed for when we close a PR without merging it.

Also, just addresses the other concerns and edited the write up :)

NathanSWard commented 3 years ago

@cart I would love any additional comments you may have about the current layout :)

cart commented 3 years ago

I think we're definitely getting closer now!

However, I just updated the PR labels to include a S-Closed for when we close a PR without merging it.

This only works if we remember to add the tag. I want to avoid "extra process" and that definitely feels like it. Additionally, every time someone closes their own pr, now a member of the triage team needs to remember to throw the label on or our entire dataset is compromised.

The triage team will [...] as well as updating the PR title to insert content-label.

I dislike this. It is redundant work on top of labels and it isn't easily queryable / structured. Additionally, I think "bug fix" vs "enhancement" is arbitrary enough that it doesn't deserve to be front-and-center in the commit log. Some bug fixes are major enhancements. Some enhancements are also bugfixes. Crude "feature" vs "fix" categories could detract from the messaging / impact of a PR. I think they're still valuable as labels, I just don't think they deserve front-and-center attention.

emojis in labels / color as category

I'm not a huge fan here either. Compare this bug label from Vue's repo: image

To this bug label from Bevy: image

The image ends up being so small that it stops being legible as a bug. I personally think color is more effective at creating associations. red==bad / bug basically everywhere (yes color perception isn't universal across cultures, but thats also true for emoji perception).

I know this throws a wrench in the "color as category" design. But I think its more useful to use color to assist in distinguishing between "bug" and "enhancement" than it is to distinguish between "bug" and "platform".

I do think I like the prefixes for category ( [T-], [C-], etc), but I personally think thats enough of a distinguisher. Using color too is redundant information.

Content [C-]

This feels more like "category" than "content".

TODO: with the priority label, I’m still not sure if we want to just tag the critical/high priority items, or to have tags for all (e.g. P0, P1, P2, P3).

I'm still very much in favor of only labeling high priority items. "purity" isn't a good enough reason to introduce a bunch of noise into the system imo. Something like 95% of issues will be "normal" priority. And using "critical" / "high" priority gives immediate context relative to, say, P1 and P2 (because users won't know how many priorities there are / what the spectrum is).

Weibye commented 3 years ago

Should there be a "Needs RFC" status tag, or is that already covered by "Needs Design"?

NathanSWard commented 3 years ago

This only works if we remember to add the tag. I want to avoid "extra process" and that definitely feels like it.

Yep that's fair. I was mostly looking for a way to distinguish closed/merged PRs, however with bors it makes this a little difficult. So leaving it off is probably best.

I dislike this. It is redundant work on top of labels and it isn't easily queryable / structured. Additionally, I think "bug fix" vs "enhancement" is arbitrary enough that it doesn't deserve to be front-and-center in the commit log.

This was referring to the content labels which is (ECS, Assets, Rendering, etc...) I do agree that it's extra work and maybe not warranted, but being able to categorize commits by just their first line is very helpful. However I've seen to have gotten quite a but of pushback from both this and the gitmoji, and I'm not longer super in favor of them, so these I'll remove.

The image ends up being so small that it stops being legible as a bug.

Yep I also agree! I actually removed these from the hackmd :wink:

I do think I like the prefixes for category ( [T-], [C-], etc), but I personally think thats enough of a distinguisher. Using color too is redundant information.

Im definitely on the side of both using the prefix T- and colors. The prefix makes it easier to search for a specific label, and also keeps the sorted alphabetically. However I would also argue the colors should stay consistent between label categories, as this helps with visual organization. Granted, while i'm on this side, I'm not against removing the color scheme, since the prefix approach is sufficient at keeping the organized. I'll probably defer to @alice-i-cecile on this, since she recommended to me the color schemes.

I'm still very much in favor of only labeling high priority items.

I don't feel strongly either way about this, however I do like how only labeling the high priority targets removes a lot of the noise :) So I'm happy with only having like critical and high priority labels :)

alice-i-cecile commented 3 years ago

Re: color, I worry that we simply have too many labels to meaningfully use color to describe the content (rather than category) of the label. I suppose we could assign colors to labels at random and then make use of them to distinguish filtered search results?

Using colors for the category helps with labelling, and guides the eye when you're trying to check for the type of information you care about.

Should there be a "Needs RFC" status tag, or is that already covered by "Needs Design"?

IMO this is covered by "needs design" + "complex".

This feels more like "category" than "content".

Totally fine by me. I just wanted a different first-letter and "content" was the first word that sprung to mind.

cart commented 3 years ago

A triage-team or something of the sort for those in change of assigning proper labels. Create a triage team?

Haha are you aware that we already have a triage team (and that you are a member)? https://github.com/orgs/bevyengine/teams/triage-team

cart commented 3 years ago

IMO this is covered by "needs design" + "complex".

agreed. although that might not be immediately apparent to some people.

Yep that's fair. I was mostly looking for a way to distinguish closed/merged PRs, however with bors it makes this a little difficult. So leaving it off is probably best.

I really hate that we need to choose between "ideal commit history" and "ideal github states / statistics". Hopefully some saint will find a way to solve this for us: https://github.com/bors-ng/bors-ng/issues/1217

However I've seen to have gotten quite a but of pushback from both this and the gitmoji, and I'm not longer super in favor of them, so these I'll remove.

Thanks :)

Using colors for the category helps with labelling, and guides the eye when you're trying to check for the type of information you care about.

Colors are fortunately the easiest thing to change (because we can do it "globally" with a single click). I'm cool with experimenting here. You both like "colors for categories" so I'm cool with trying that first (although I do think throwing away red for bugs is a major loss given how common that is / how strong those associations are).

alice-i-cecile commented 3 years ago

although that might not be immediately apparent to some people.

Yeah. That said, I expect that we should basically never be dropping the needs-rfc label on others work without any explicit discussion about why (and links to the RFC process for new contributors).

cart commented 3 years ago

Good point!

NathanSWard commented 3 years ago

As was brought up in #2277 , @alice-i-cecile should we have a content label per crate inside crates/?

mockersf commented 3 years ago

As was brought up in #2277 , @alice-i-cecile should we have a content label per crate inside crates/?

Not Alice, but I think so! Would it be possible to add labels automatically based on files changed?

alice-i-cecile commented 3 years ago

I'd love this if it was automatic.

NathanSWard commented 3 years ago

Would it be possible to add labels automatically based on files changed?

I'll take a look into this and see if it's possible :)

NathanSWard commented 3 years ago

@alice-i-cecile @mockersf Looks like this might be able to automate labels depending on which files are edited. https://github.com/actions/labeler

cart commented 3 years ago

I'm not sold on automatic labels yet. Something like a "new cargo fmt rule" would touch everything / add a ton of labels automatically.

I trust people's ability to categorize things more than automation and I think that will produce a better dataset.

cart commented 3 years ago

^ forgot to scope this comment specifically to "automatic labels by file path"

alice-i-cecile commented 3 years ago

I wonder if we could automatically copy (relevant) labels from linked issues over to the PR 🤔 That's one of the most common patterns, and would improve the starting quality of new PRs a fair bit. Ah well, it's not too bad to do manually.

NathanSWard commented 3 years ago

I trust people's ability to categorize things more than automation and I think that will produce a better dataset.

Yep that's a fair sentiment. It just nice to know that if this is something we eventually want it is possible.

NathanSWard commented 3 years ago

Ohh on second thought, since there is no way via GitHub to automatically add labels to PRs (afaik) we could use this to just automatically add the needs triage label to any file aka "*" 🤔

bjorn3 commented 3 years ago

Ohh on second thought, since there is no way via GitHub to automatically add labels to PRs (afaik)

You can use the pull_request_target trigger which runs the master workflow and has a writable github token.

NathanSWard commented 3 years ago

Hey all, I would like to finalize the new labeling scheme that is proposed in the referenced HackMD.

Specifically for the category labels I tried to have one per sub-crate in the crates/ directory (while I grouped a few of them together).

Please let me know if you have any major concerns with the current proposal so we can iron out the edges here quickly <3

cart commented 3 years ago

Yup I think we're in a pretty good spot now. Just a few small things:

mockersf commented 3 years ago

about labels and colors: there are a few that have special meaning in github (top of my head: green/help wanted, purple/good first issue, red/bug) that would be good to keep as standard github

cart commented 3 years ago

Agreed. Removing those common colors throws away a lot of useful, already-established context. I also still think using color to distinguish between things within a category is more important than using it to distinguish categories (especially when we already have prefixes for categories). (bug vs enhancement) is way more relevant than (bug vs windows) and there are only 6 categories/types, whereas there are 14 [T] labels and 18 [C] labels.

NathanSWard commented 3 years ago

I think docs and examples are different enough that they merit their own Type

Sounds good, I separated them out into their own labels

Removing those common colors throws away a lot of useful, already-established context. I also still think using color to distinguish between things within a category is more important than using it to distinguish categories

Sure, I'm not totally against this scheme. While I personally would prefer the color coordination within categories, I don't feel super strongly about this, so I'm happy to have specific labels (e.g. T-bug) keep specific colors. My only question is then for the other labels that we don't have strong opinions about color, e.g. T-performance how do we manage that color? Either 1) Random 2) Color coordinate each category and then have the select number of outliers stand out with specific colors. 3) Have each category pick a color pattern. e.g. T- labels can use warm colors, or more specifically reds/oranges/brown etc... and then platform can use green/cool colors?

My preference is probably 3 (as we have some color coordination, but it's less strict. :smile:

We should come up with a migration strategy to convert old labels to new labels / preserve existing triage work.

I think the best strategy is: 1) modify existing labels (that have matching new labels) with the appropriate name and prefix e.g. needs-triage -> S-needs-triage meta -> C-ecosystem ready-for-cart -> S-ready-for-final-reveiw

2) add new labels that don't exist yet

3) Migrate all labels that we are removing to the new appropriate label

4) remove old labels

Weibye commented 3 years ago

Should there be category labels for

?

NathanSWard commented 3 years ago

Should there be category labels for

Added :)

alice-i-cecile commented 3 years ago

3 is good color scheme!

Do we have a good "getting to hello world" label? We collect a bunch of those and they'd be nice to deduplicate easily.

cart commented 3 years ago

(3) is a good idea, but it still feels like we're throwing out too many color options for each category (there are a ton of labels in each category and we will run out of "warm" colors fast). Maybe we could use "lightness/saturation level" instead? T- could be "light / saturated colors", C- could be "dark / desaturated colors", P- could be fully desaturated (maybe just greyscale ... there aren't that many platforms)

Weibye commented 3 years ago

Tried sketching out some examples with the primary goal of keeping github's existing colors. In this effort having 1 theme per category wasn't entirely possible when it comes to 'warm' colors or similar, but each category has for the most part a similar level of luminance and saturation.

My findings so far is:

Scheme 01

image

image

image

mockersf commented 3 years ago

for colors, I would prefer everything in a category has the same color, except for existing label colors in GitHub universe that would keep their own

Maybe the tiniest of variation, but instead of warm, saturated, ... I would go for shades of blue, ...

bjorn3 commented 3 years ago

Rust uses O- for platforms, C- for the category (bug, enhancement, cleanup, ...), I- for things like crash, hang, unsoundness and A- for the area (ecs, sound, ui, ...)

If C- and I- are kept in a single category I would prefer the C- prefix over T-. The later is used by rust to mean to which team it applies. Using it in bevy for something else would be slightly confusing IMHO at least to me.

cart commented 3 years ago

Yeah I'm down to align with rust here according to the "letter changes" @bjorn3 laid out.

As far as colors go, I think the scheme @Weibye laid out doesn't have a clear enough distinction between the categories, largely because it tries to re-use colors multiple times within a category (with varying levels of lightness + saturation) and avoids re-using colors across categories. It feels like a "compromise" between "unique colors per category" and "varying lightness per category" that trades off at-a-glace category recognition.

I'll let people propose more color alternatives for about a day, then I'll probably just make a call. No need to block moving forward on picking a palette. We can easily change colors later.

Weibye commented 3 years ago

I think the scheme @Weibye laid out doesn't have a clear enough distinction between the categories,

I'll try putting up some more color examples in the morning

Weibye commented 3 years ago

Rust-alike labels (scheme 02)

This color-scheme follows what is found in the rust-lang github to a reasonable degree.

Notable changes from current Hackmd document:

Unresolved questions:

Rust-alike labels visualized, (expand to view) ![image](https://user-images.githubusercontent.com/13300393/121493490-2c670500-c9d8-11eb-8b0d-f1d52fbb35bf.png)

Rust-alike with existing colors (scheme 03)

Same as above but keeping existing colors for select labels.

Rust-alike labels, with existing colors visualized, (expand to view) ![image](https://user-images.githubusercontent.com/13300393/121493723-63d5b180-c9d8-11eb-8784-3e51317b7c9b.png)

Apologies for the tall images, github does not like inline-style in .md documents.

Edit: I'll add a third example later during the day that should incorporate the concept of 'themes' a bit more, mostly to serve as a backdrop for conversation.

NathanSWard commented 3 years ago

This color-scheme follows what is found in the rust-lang github to a reasonable degree.

Wow this looks really good!! :D

The only comment I have I'd I don't think we need the Issues labels. I-Needs-Decision I don't think really provides us with anything. I-Crash shouldn't be limited to issues as someone could open up a PR that fixes some crash in bevy. In which case we'd want the Crash label there as well.

Weibye commented 3 years ago

The only comment I have I'd I don't think we need the Issues labels. I-Needs-Decision I don't think really provides us with anything. I-Crash shouldn't be limited to issues as someone could open up a PR that fixes some crash in bevy. In which case we'd want the Crash label there as well.

Yeah I completely agree with you. Here, have some more

Scheme 04: Rust-alike gradients

While it looks very pretty, my primary concern is maintenance. When adding new labels to a category, who would be responsible to pick a new color and how much friction would that entail? (compared to just having fixed colors for the entire group)

Also, lables like C-Bug not being red is really not great for readability.

Expand to view ![image](https://user-images.githubusercontent.com/13300393/121588911-c1005000-ca36-11eb-9dc4-f40cb18c319d.png) ![image](https://user-images.githubusercontent.com/13300393/121588981-d6757a00-ca36-11eb-890c-d2f93172f69a.png)

Scheme 05: Rust-alike gradients with existing colors

Keeping with the gradients but retaining existing colors. Followed @NathanSWard suggestion to scrap the I-Issue category and moving them to C-Category instead.

When it comes to the existing colors, I would like to suggest not keeping E-Good-First-Issue's existing color, but replacing it with green as seen in scheme 04.

I really do like that red is a very exclusive color in this scheme, which could be put onto other labels we feel strongly about.

Expand to view ![image](https://user-images.githubusercontent.com/13300393/121592050-84365800-ca3a-11eb-9344-c9afa04f477d.png)

To sum up

I personally feel scheme 05 could be the best alternative (or a non-gradient version of it), with a few tweaks as desired.

mockersf commented 3 years ago

I prefer 05 without gradient 👍

Weibye commented 3 years ago

Scheme 06

This is a non-gradient version of scheme 05.

This is the colors for each group, with exceptions for the labels that keep their existing color and does not match with the rest of the group. image

Existing colors

Label Status
C-Bug Keeps its color and becomes outlier in group
C-Crash Keeps its color and becomes outlier in group
C-Invalid Keeps its color and becomes outlier in group
E-Help-Wanted Keeps its existing color and the color is adopted to the whole group
S-Needs-Investigation Keeps its existing color and the color is adopted to the whole group
C-Docs Gets new color from the group
E-Good-First-Issue Gets new color from the group
S-Wontfix Gets new color from the group
C-Duplicate Gets new color from the group
Expand to view full scheme ![image](https://user-images.githubusercontent.com/13300393/121644475-53364180-ca93-11eb-95bd-cbfe299bfbac.png)

Thoughts

Note: Again, any issues related to readability of the colors should be a non-issue as github label system picks a foreground-background color pair that ensures readability

Weibye commented 3 years ago

This is what it would look like as labels (using scheme 06)

image

NathanSWard commented 3 years ago

Scheme 06

Yep! This looks great! :) The only thing I would add is that C-Hotfix should have its own color (since hotfixes are usually pretty crucial)

TheRawMeatball commented 3 years ago

We should probably have a uses-unsafe label for PR s