Remove get_unchecked comment #2925
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
# Objective - Related to #2514 - not sure if it's a proper fix long term - CI was complaining Error: Path `"/usr/local/lib/android/sdk/ndk-bundle/toolchains/llvm/prebuilt/linux-x86_64/bin/aarch64-linux-android31-clang"` was not found - A lot of questions started popping up 10 days ago about ["android build tools 31 corrupted"](https://www.google.com/search?q=android+build+tools+31+corrupted) - https://stackoverflow.com/questions/68387270/android-studio-error-installed-build-tools-revision-31-0-0-is-corrupted - Uninstalling `"build-tools;31.0.0"` doesn't seem to work, as it removes other components even though `"build-tools;30.0.3"` are available ## Solution - Uninstalling `"platforms;android-31"` seems to do the trick and `cargo-apk` stops trying to target `...31` Android is not my thing, this solution was found with a lot of trials and errors. I am not sure how long term it is, I don't know the release schedule of android build tools, or if we need to target this 31 thing. I just wanted to stop all those failed ci everywhere...
# Objective - There are a few random failures in CI, mostly due to contacting crates.io or checking for deadlinks - CI can take some time, more than 20 minutes for a full status - A clippy/format issue stops running tests on other platforms ## Solution - Use GitHub cache for cargo artefacts - This speeds up builds and reduce dependencies on outside world - Reorder and add dependencies between short jobs. They are still setup to run even if one of the dependency failed - This reduce the number of parallel jobs that are running for one PR. On GitHub free tier, we're limited to 20. - Split CI checks (format & clippy) in its own job - This speeds up test jobs, and allow us to not kill all platform tests for a format issue - Retry in case of dead links check failure - Internet is just that kind of place where things may seem dead at some point but back alive 5 seconds later ## Before <img width="1062" alt="Screenshot 2021-07-27 at 01 18 52" src="https://user-images.githubusercontent.com/8672791/127071973-9a2c5ce8-c871-4f8d-9b17-08871824b6c4.png"> ## After (with all cache live) <img width="1063" alt="Screenshot 2021-07-27 at 01 18 28" src="https://user-images.githubusercontent.com/8672791/127071986-767a7e65-53ed-45fd-8d75-51a571f0b851.png">
This is extracted out of eb8f973 and includes some additional changes to remove all references to AppBuilder and fix examples that still used App::build() instead of App::new(). In addition I didn't extract the sub app feature as it isn't ready yet. You can use `git diff --diff-filter=M eb8f973` to find all differences in this PR. The `--diff-filtered=M` filters all files added in the original commit but not in this commit away. Co-Authored-By: Carter Anderson <mcanders1@gmail.com>
# Objective While looking at the code of `World`, I noticed two basic functions (`get` and `get_mut`) that are probably called a lot and with simple code that are not `inline` ## Solution - Add benchmark to check impact - Add `#[inline]` ``` group this pr main ----- ---- ---- world_entity/50000_entities 1.00 115.9±11.90µs ? ?/sec 1.71 198.5±29.54µs ? ?/sec world_get/50000_entities_SparseSet 1.00 409.9±46.96µs ? ?/sec 1.18 483.5±36.41µs ? ?/sec world_get/50000_entities_Table 1.00 391.3±29.83µs ? ?/sec 1.16 455.6±57.85µs ? ?/sec world_query_for_each/50000_entities_SparseSet 1.02 121.3±18.36µs ? ?/sec 1.00 119.4±13.88µs ? ?/sec world_query_for_each/50000_entities_Table 1.03 13.8±0.96µs ? ?/sec 1.00 13.3±0.54µs ? ?/sec world_query_get/50000_entities_SparseSet 1.00 666.9±54.36µs ? ?/sec 1.03 687.1±57.77µs ? ?/sec world_query_get/50000_entities_Table 1.01 584.4±55.12µs ? ?/sec 1.00 576.3±36.13µs ? ?/sec world_query_iter/50000_entities_SparseSet 1.01 169.7±19.50µs ? ?/sec 1.00 168.6±32.56µs ? ?/sec world_query_iter/50000_entities_Table 1.00 26.2±1.38µs ? ?/sec 1.91 50.0±4.40µs ? ?/sec ``` I didn't add benchmarks for the mutable path but I don't see how it could hurt to make it inline too...
# Objective - Remove all the `.system()` possible. - Check for remaining missing cases. ## Solution - Remove all `.system()`, fix compile errors - 32 calls to `.system()` remains, mostly internals, the few others should be removed after #2446
self explanatory
…ponents and real rust types (#2490) # Objective There is currently a 1-to-1 mapping between components and real rust types. This means that it is impossible for multiple components to be represented by the same rust type or for a component to not have a rust type at all. This means that component types can't be defined in languages other than rust like necessary for scripting or sandboxed (wasm?) plugins. ## Solution Refactor `ComponentDescriptor` and `Bundle` to remove `TypeInfo`. `Bundle` now uses `ComponentId` instead. `ComponentDescriptor` is now always created from a rust type instead of through the `TypeInfo` indirection. A future PR may make it possible to construct a `ComponentDescriptor` from it's fields without a rust type being involved.
on nightly these two clippy lints fail: - [needless_borrow](https://rust-lang.github.io/rust-clippy/master/#needless_borrow) - [unused_unit](https://rust-lang.github.io/rust-clippy/master/#unused_unit)
# Objective notify 5.0.0-pre.11 breaks the interface again, but apparently in a way that's similar to how it used to be ## Solution Bump `bevy_asset` dependency on notify to `5.0.0-pre.11` and fix the errors that crop up. It looks like `pre.11` was mentioned in #2528 by @mockersf but there's no mention of why `pre.10` was chosen ultimately.
It doesn't compile on wasm, and it's full of footguns # Objective - If bevy is used with default features on wasm, there's more of a chance it will compile - Note that I haven't done a full audit - it's possible that there are other problematic crates ## Solution - `bevy_dynamic_plugin` is no longer a default plugin - I've also done an accidental drive by reformatting of the root `Cargo.toml`, as I have [Even Better Toml](https://github.com/tamasfe/taplo) installed. - (Please, rustfmt do this for us)
there was a typo 👀 i fixed it 👀
# Objective - #2551 revamped our CI setup which included running clippy and rustfmt in another Job. - This new Job wasn't added to the bors.toml, which means that PRs would be accepted that didn't run them. ## Solution - Add the "ci" job to the bors.toml
…2604) ## Objective This code would result in a crash: ```rust use bevy::prelude::*; fn main() { let mut world = World::new(); let child = world.spawn().id(); world.spawn().push_children(&[child]); } ``` ## Solution Update the `EntityMut`'s location after inserting a component on the children entities, as it may have changed.
# Objective Fixes #2620 ## Solution Remove WithBundle filter and temporarily remove example for query_bundle.
# Objective
- Allow `ScheduleRunnerPlugin` to be instantiated without curly braces. Other plugins in the library already use the semicolon syntax.
- Currently, you have to do the following:
```rust
App::build()
.add_plugin(bevy::core::CorePlugin)
.add_plugin(bevy::app::ScheduleRunnerPlugin {})
```
- With the proposed change you can do this:
```rust
App::build()
.add_plugin(bevy::core::CorePlugin)
.add_plugin(bevy::app::ScheduleRunnerPlugin)
```
## Solution
- Change the `ScheduleRunnerPlugin` definition to use a semicolon instead of curly braces.
I didn't know about MinimalPlugins for way too long. This should increase visibility for others. # Objective Improve visibility and discover in the docs for Default and Minimal Plugins. ## Solution Links the two Docs pages. Co-authored-by: Mirko Rainer <52899592+mirkoRainer@users.noreply.github.com>
# Objective Prevent some possible problem Found by IntelliJ IDEA ## Solution Double quoting variable and exit on possible failure of cd command.
# Objective - Provides more useful error messages when using unsupported shader features. ## Solution Fixes #869 - Provided a error message as follows (adding name, set and binding): ``` Unsupported shader bind type CombinedImageSampler (name noiseVol0, set 0, binding 9) ```
## Objective - Clean up remaining references to the trait `FromResources`, which was replaced in favor of `FromWorld` during the ECS rework. ## Solution - Remove the derive macro for `FromResources` - Change doc references of `FromResources` to `FromWorld` (this is the first item in #2576)
# Objective
This:
```rust
use bevy::prelude::*;
fn main() {
App::new()
.add_system(test)
.run();
}
fn test(entities: Query<Entity>) {
let mut combinations = entities.iter_combinations_mut();
while let Some([e1, e2]) = combinations.fetch_next() {
dbg!(e1);
}
}
```
fails with the message "the trait bound `bevy::ecs::query::EntityFetch: std::clone::Clone` is not satisfied".
## Solution
It works after adding the naive clone implementation to EntityFetch. I'm not super familiar with ECS internals, so I'd appreciate input on this.
[**RENDERED**](https://github.com/alice-i-cecile/bevy/blob/better-contributing/CONTRIBUTING.md) Improves #910. As discussed in #1309, we'll need to synchronize content between this and the Bevy website in some way (and clean up the .github file perhaps?). I think doing it as a root-directory file is nicer for discovery, but that's a conversation I'm interested in having. This document is intended to be helpful to beginners to open source and Bevy, and captures what I've learned about our informal practices and values. Reviewers: I'm particularly interested in: - opinions on the items **What we're trying to build**, where I discuss some of the project's high-level values and goals - more relevant details on the `bevy` subcrates for **Getting oriented** - useful tricks and best practices that I missed - better guidance on how to contribute to the Bevy book from @cart <3
I don't see much of a reason at this point to boost my name over anyone elses. We are all Bevy Contributors.
# Objective Enable using exact World lifetimes during read-only access . This is motivated by the new renderer's need to allow read-only world-only queries to outlive the query itself (but still be constrained by the world lifetime). For example: https://github.com/bevyengine/bevy/blob/115b170d1f11a91146bb6d6e9684dceb8b21f786/pipelined/bevy_pbr2/src/render/mod.rs#L774 ## Solution Split out SystemParam state and world lifetimes and pipe those lifetimes up to read-only Query ops (and add into_inner for Res). According to every safety test I've run so far (except one), this is safe (see the temporary safety test commit). Note that changing the mutable variants to the new lifetimes would allow aliased mutable pointers (try doing that to see how it affects the temporary safety tests). The new state lifetime on SystemParam does make `#[derive(SystemParam)]` more cumbersome (the current impl requires PhantomData if you don't use both lifetimes). We can make this better by detecting whether or not a lifetime is used in the derive and adjusting accordingly, but that should probably be done in its own pr. ## Why is this a draft? The new lifetimes break QuerySet safety in one very specific case (see the query_set system in system_safety_test). We need to solve this before we can use the lifetimes given. This is due to the fact that QuerySet is just a wrapper over Query, which now relies on world lifetimes instead of `&self` lifetimes to prevent aliasing (but in systems, each Query has its own implied lifetime, not a centralized world lifetime). I believe the fix is to rewrite QuerySet to have its own World lifetime (and own the internal reference). This will complicate the impl a bit, but I think it is doable. I'm curious if anyone else has better ideas. Personally, I think these new lifetimes need to happen. We've gotta have a way to directly tie read-only World queries to the World lifetime. The new renderer is the first place this has come up, but I doubt it will be the last. Worst case scenario we can come up with a second `WorldLifetimeQuery<Q, F = ()>` parameter to enable these read-only scenarios, but I'd rather not add another type to the type zoo.
# Objective While implementing a plugin for my rollback networking library, I needed to load/save parts of the world. For this, I made a WorldSnapshot that works quite like the current DynamicScene. Using a TypeRegistry to register component types I want to save/load and then using ReflectComponents methods to add or apply components of the given types. However, I noticed there is no method to remove components from entities through the ReflectComponent. ## Solution I added a `remove_component` field to the `ReflectComponent` struct, as well as a `pub fn remove_component(&self, world: &mut World, entity: Entity)` to call that function in `remove_component`. This follows exactly the same pattern all other methods/fields in this struct look like. This is an example how it could be used (at least how I would use it): https://github.com/gschup/bevy_ggrs/blob/6c003f86f1993bcbb21c180fab2e8ef664b7f7c9/src/world_snapshot.rs#L133
# Objective - We currently depends on ndk 0.2, 0.3, 0.4 - Only 0.2 dependencies comes from Bevy itself ## Solution - Replace #1371 - Update Bevy to ndk-glue 0.4 - Also fixes duplicate dependency CI issue
Updates the requirements on [glam](https://github.com/bitshifter/glam-rs) to permit the latest version. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/bitshifter/glam-rs/blob/master/CHANGELOG.md">glam's changelog</a>.</em></p> <blockquote> <h2>[0.17.3] - 2021-07-18</h2> <h3>Fixed</h3> <ul> <li>Fix alignment unit tests on non x86 platforms.</li> </ul> <h2>[0.17.2] - 2021-07-15</h2> <h3>Fixed</h3> <ul> <li>Fix alignment unit tests on i686 and S390x.</li> </ul> <h2>[0.17.1] - 2021-06-29</h2> <h3>Added</h3> <ul> <li>Added <code>serde</code> support for <code>Affine2</code>, <code>DAffine2</code>, <code>Affine3A</code> and <code>DAffine3</code>.</li> </ul> <h2>[0.17.0] - 2021-06-26</h2> <h3>Breaking changes</h3> <ul> <li>The addition of <code>Add</code> and <code>Sub</code> implementations of scalar values for vector types may create ambiguities with existing calls to <code>add</code> and <code>sub</code>.</li> <li>Removed <code>From<Mat3></code> implementation for <code>Mat2</code> and <code>From<DMat3></code> for <code>DMat2</code>. These have been replaced by <code>Mat2::from_mat3()</code> and <code>DMat2::from_mat3()</code>.</li> <li>Removed <code>From<Mat4></code> implementation for <code>Mat3</code> and <code>From<DMat4></code> for <code>DMat3</code>. These have been replaced by <code>Mat3::from_mat4()</code> and <code>DMat3::from_mat4()</code>.</li> <li>Removed deprecated <code>from_slice_unaligned()</code>, <code>write_to_slice_unaligned()</code>, <code>from_rotation_mat4</code> and <code>from_rotation_ypr()</code> methods.</li> </ul> <h3>Added</h3> <ul> <li>Added <code>col_mut()</code> method which returns a mutable reference to a matrix column to all matrix types.</li> <li>Added <code>AddAssign</code>, <code>MulAssign</code> and <code>SubAssign</code> implementations for all matrix types.</li> <li>Added <code>Add</code> and <code>Sub</code> implementations of scalar values for vector types.</li> <li>Added more <code>glam_assert!</code> checks and documented methods where they are used.</li> <li>Added vector projection and rejection methods <code>project_onto()</code>, <code>project_onto_normalized()</code>, <code>reject_from()</code> and <code>reject_from_normalized()</code>.</li> <li>Added <code>Mat2::from_mat3()</code>, <code>DMat2::from_mat3()</code>, <code>Mat3::from_mat4()</code>, <code>DMat3::from_mat4()</code> which create a smaller matrix from a larger one, discarding a final row and column of the input matrix.</li> <li>Added <code>Mat3::from_mat2()</code>, <code>DMat3::from_mat2()</code>, <code>Mat4::from_mat3()</code> and <code>DMat4::from_mat3()</code> which create an affine transform from a smaller linear transform matrix.</li> </ul> <h3>Changed</h3> <!-- raw HTML omitted --> </blockquote> <p>... (truncated)</p> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/bitshifter/glam-rs/commit/ecf3904b2fb639a979127f457b259f4b83b0340b"><code>ecf3904</code></a> Prepare release 0.17.3</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/95e02bb43eadbc0c56b3931cbbf1f68a5cc5f910"><code>95e02bb</code></a> Merge branch 'master' of github.com:bitshifter/glam-rs</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/c6dc70258345349de5f1051d19ce1f408532ca89"><code>c6dc702</code></a> More alignment test fixes for when SSE2 is not avaialable.</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/87a3b25872f893fc4e8cbab0bbfbb437d1a2c8fb"><code>87a3b25</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/bitshifter/glam-rs/issues/216">#216</a> from bitshifter/prepare-0.17.2</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/269e5140903f9bf5bcaa8073274f4a0113d7e0cd"><code>269e514</code></a> Prepare for 0.17.2 release.</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/1da7d6459c5f9890275c7fc1871e6c33c825b6b9"><code>1da7d64</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/bitshifter/glam-rs/issues/215">#215</a> from bitshifter/issue-213</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/dc60e20925254f4f1239424685c34953409395df"><code>dc60e20</code></a> Fix align asserts on i686 and S390x architectures.</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/bd8b30e9fbcfd5910dc2af251fecccc31670edf7"><code>bd8b30e</code></a> Merge pull request <a href="https://github-redirect.dependabot.com/bitshifter/glam-rs/issues/212">#212</a> from remilauzier/master</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/a4e97c0b5429547718a9ceacfe5957d1d765bd52"><code>a4e97c0</code></a> Update approx to 0.5</li> <li><a href="https://github.com/bitshifter/glam-rs/commit/059f61952533af3fc32274e5b7ea242ce9a771a7"><code>059f619</code></a> Prepare 0.17.1 release (<a href="https://github-redirect.dependabot.com/bitshifter/glam-rs/issues/211">#211</a>)</li> <li>Additional commits viewable in <a href="https://github.com/bitshifter/glam-rs/compare/0.15.1...0.17.3">compare view</a></li> </ul> </details> <br /> Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) </details>
See the individual commits.
Just fixes does a quick fix while I was hunting for TODO's
alice-i-cecile
added
C-Code-Quality
alice-i-cecile
approved these changes
Oct 7, 2021
DJMcNab
reviewed
Oct 7, 2021
| @@ -87,10 +87,21 @@ impl From<image::DynamicImage> for Texture { | |||
| Vec::with_capacity(width as usize * height as usize * format.pixel_size()); | |||
|
|
|||
| for pixel in image.into_raw().chunks_exact(3) { | |||
| // TODO unsafe_get in release builds? | |||
| let r = pixel[0]; | |||
There was a problem hiding this comment.
Are there actually bound checks anyway?
I'm fairly certain they'd be optimised out; that's what my searching of discussion of exact_chunks yields.
| @@ -87,10 +87,21 @@ impl From<image::DynamicImage> for Texture { | |||
| Vec::with_capacity(width as usize * height as usize * format.pixel_size()); | |||
|
|
|||
| for pixel in image.into_raw().chunks_exact(3) { | |||
| // TODO unsafe_get in release builds? | |||
There was a problem hiding this comment.
A better TODO would be to use the array_chunks method once stabilised
|
Oh okay, I'll just snip this PR then. |
Could you swap over the TODO comment and then we can merge this? :) |
|
Yeah, exactly. This PR is well placed to change that TODO. |
|
So just to confirm, I just swap the todo comment and use the original code. |
|
Yeah, exactly that please. |
|
Could you also change this PR to target branch |
|
Hey sorry about being slow, I'm currently not in the United States and the wifi has been slow at best (8kb/s at peak :( ). I'll implement the changes and push very shortly |
|
@mockersf, how would I target the pipleined-rendering branch? Sorry to ask, I'm not crazy familiar with advanced PR stuff |
|
You should also change the name of this PR, so then the commit history will make sense when merged :) |
billyb2
changed the title
|
So I changed the base, and now all of those commits are there... |
|
@billyb2 sounds good. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just fixes does a quick fix while I was hunting for TODO's
Objective
Fixes a TODO that requests to use get_unchecked during release builds, rather than indexing normally (which includes bounds checking).
Solution
Used some conditional compilation that uses the regular indexing method on debug builds, and uses the
get_uncheckedmethod on release builds.