Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use wildcard imports in bevy_pbr #9847

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

nicopap
Copy link
Contributor

@nicopap nicopap commented Sep 19, 2023

Objective

  • the style of import used by bevy guarantees merge conflicts when any file change
  • This is especially true when import lists are large, such as in bevy_pbr
  • Merge conflicts are tricky to resolve. This bogs down rendering PRs and makes contributing to bevy's rendering system more difficult than it needs to

Solution

  • Use wildcard imports to replace multiline import list in bevy_pbr

I suspect this is controversial, but I'd like to hear alternatives. Because this is one of many papercuts that makes developing render features near impossible.

@nicopap nicopap added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR labels Sep 19, 2023
@alice-i-cecile
Copy link
Member

Hmm. I don't mind it. Ideally it would be possible to create a tool which detects and automatically resolves and formats trivial import merge conflicts like this.

Perhaps a GitHub bot like bors? It could even run fully automatically.

I agree that these are a major paper it to contributing, both in and outside of rendering so I'm enthusiastic about improving the situation by whatever means we have.

@nicopap
Copy link
Contributor Author

nicopap commented Sep 19, 2023

I think the "ideal" would be some sort of subset feature preludes. In the case of rendering, it's especially egregious because there are so many wgpu-related types. I think it makes sense tbh to always import render_resource items as a wildcard import.

@nicopap
Copy link
Contributor Author

nicopap commented Sep 19, 2023

In my own crates, I've been experimenting with the idea of never having multiline imports. It's purely esthetic preference, since I'm the only one working on them, it's difficult to tell if it is any help.

@nicopap
Copy link
Contributor Author

nicopap commented Sep 19, 2023

The other "ideal" solution would be to configure rustfmt to have single item per line.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 19, 2023
@JMS55
Copy link
Contributor

JMS55 commented Sep 19, 2023

I'm in favor of this, but:

  • We should configure a rustfmt config for import style. Currently I've been choosing between stuff like crate:: vs super::, whether to put newlines between imports from different crates, etc according to personal preference. Ideally this is made consistent across bevy automatically, so I no longer have to think about it or manually change imports.
  • We should probably hold off on merging this until Automatic batching/instancing of draw commands #9685

@nicopap nicopap removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 26, 2023
@nicopap nicopap added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Sep 27, 2023
@nicopap nicopap force-pushed the no-conflict-import branch from 741ad2b to 9131335 Compare October 25, 2023 08:16
@nicopap nicopap removed the X-Controversial There is active debate or serious implications around merging this PR label Oct 25, 2023
@mockersf
Copy link
Member

Not controversial for me. This change has no impact except on contributor experience, and can be changed back if it doesn't work for us in a few minutes with an IDE.

It's becoming more painful to contribute on the renderer, we should merge this soon

@superdump superdump added this pull request to the merge queue Oct 25, 2023
Merged via the queue into bevyengine:main with commit 66f72dd Oct 25, 2023
21 checks passed
@nicopap nicopap deleted the no-conflict-import branch October 25, 2023 09:04
@JMS55 JMS55 mentioned this pull request Oct 28, 2023
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- the style of import used by bevy guarantees merge conflicts when any
file change
- This is especially true when import lists are large, such as in
`bevy_pbr`
- Merge conflicts are tricky to resolve. This bogs down rendering PRs
and makes contributing to bevy's rendering system more difficult than it
needs to

## Solution

- Use wildcard imports to replace multiline import list in `bevy_pbr`

I suspect this is controversial, but I'd like to hear alternatives.
Because this is one of many papercuts that makes developing render
features near impossible.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- the style of import used by bevy guarantees merge conflicts when any
file change
- This is especially true when import lists are large, such as in
`bevy_pbr`
- Merge conflicts are tricky to resolve. This bogs down rendering PRs
and makes contributing to bevy's rendering system more difficult than it
needs to

## Solution

- Use wildcard imports to replace multiline import list in `bevy_pbr`

I suspect this is controversial, but I'd like to hear alternatives.
Because this is one of many papercuts that makes developing render
features near impossible.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants