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

disable bevy default features #70

Merged
merged 2 commits into from
Dec 14, 2020
Merged

disable bevy default features #70

merged 2 commits into from
Dec 14, 2020

Conversation

mockersf
Copy link
Contributor

this speeds up compile time, and allow usage of this library in a wasm build

@aevyrie
Copy link
Owner

aevyrie commented Dec 14, 2020

Looks like a small change in master caused a test failure in multiple_windows on lines 42/43 (add a ".0" to make window size a float).

Does this change have any other implications you're aware of? I'm not well versed in the use of cargo features. I assume all this is doing is minimizing the number of feature dependencies, which is useful in applications like wasm.

@mockersf
Copy link
Contributor Author

It doesn't change anything except reducing the number of dependencies for applications that are already limiting the number of dependencies

The important (and undocumented) thing about rust features is that cargo treats them as additive, so it will use the union of all features enabled to a dependency. you can't have mutually exclusive features, you can't have features enabled for one transitive dependency and not for the other (ie if I depend on bevy and on bevy_mod_picking which depends on bevy, bevy will be compiled with the union of enabled features from both dependencies).

I tested this PR in a wasm build and it works 👍

@aevyrie
Copy link
Owner

aevyrie commented Dec 14, 2020

Thanks for the explanation - really appreciate that! Would you mind either:

  • including the small fix to resolve the failed test
  • or, pointing to a previous bevy git commit in the cargo.toml, prior to this breaking change

I can then merge in this PR. Thanks!

@mockersf
Copy link
Contributor Author

test fixed!

@aevyrie aevyrie merged commit ccee08f into aevyrie:master Dec 14, 2020
@aevyrie
Copy link
Owner

aevyrie commented Dec 15, 2020

Turns out this is causing a runtime panic for the examples:

thread 'thread 'mainthread '' panicked at 'Compute Task Pool (1)Resource does not exist alloc::boxed::Box<dyn bevy_render::renderer::render_resource_context::RenderResourceContext>.' panicked at 'Compute Task Pool (3)', Resource does not exist alloc::boxed::Box<dyn bevy_render::renderer::render_resource_context::RenderResourceContext>.C:\Users\aevyr\.cargo\git\checkouts\bevy-f7ffde730c324c74\45e2be3\crates\bevy_ecs\src\resource\resources.rs', ' panicked at 'C:\Users\aevyr\.cargo\git\checkouts\bevy-f7ffde730c324c74\45e2be3\crates\bevy_ecs\src\resource\resources.rs::Resource does not exist alloc::boxed::Box<dyn bevy_render::renderer::render_resource_context::RenderResourceContext>.282282::32',
32C:\Users\aevyr\.cargo\git\checkouts\bevy-f7ffde730c324c74\45e2be3\crates\bevy_ecs\src\resource\resources.rsnote: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
:
282:32
thread 'main' panicked at 'Task thread panicked while executing.: Any', C:\Users\aevyr\.cargo\git\checkouts\bevy-f7ffde730c324c74\45e2be3\crates\bevy_tasks\src\task_pool.rs:75:18

I think this means we need to figure out which dependency is needed, then add it as a required feature for examples.

@mockersf
Copy link
Contributor Author

sorry about that, it's done in #71

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants