Skip to content
This repository has been archived by the owner on Nov 29, 2022. It is now read-only.

Use Separate Cargo Features for Debug 2D and 3D #118

Merged
merged 4 commits into from Jun 25, 2021

Conversation

zicklag
Copy link
Contributor

@zicklag zicklag commented Jun 22, 2021

Currently, the heron crate doesn't expose anything unless either the 2D or 3D features are enabled, but enabling either of those features also enables the debug renderer. This makes it impossible to use heron without enabling the debug rendering crates.

My projects use Bevy Retrograde, which uses a custom renderer on top of Bevy that won't compile when bevy's rendering features are enabled. Because the debug crates require Bevy's rendering features, there's no combination of features under which I can compile heron for use in Bevy Retrograde.

This change makes it required to pass in the debug-2d or debug-3d features in order to actually include the heron_debug. This allows me to run my project without including bevy's rendering features.

I'm not sure this is the best way to do it and I'm totally open to any other options, as long as there is some way to compile without needing heron_debug.

@jcornaz
Copy link
Owner

jcornaz commented Jun 23, 2021

Good catch, thanks.

I realized recently that the model I use for cargo feature doesn't fly. And it especially bothers me that heron violates recomandations of the official cargo book: "features should be additive" and "Enabling a feature should not introduce a SemVer-incompatible change".

I want to rework the features of heron to have only 2 features:

  • 3d
  • debug

There wouldn't be any 2d features, and none of the feature above would be enabled by default.

That means the usage will be as following:

  • 2d without debug: [] (the default)
  • 2d with debug: ["debug"]
  • 3d without debug: ["3d"]
  • 3d with debug: ["3d", "debug"]

That's the best slution I can propose so far. If you want to try it yourself in this PR you can. But you don't have to ;-). I'll try to find time this week to do this change.

@jcornaz jcornaz self-requested a review June 23, 2021 07:03
@jcornaz jcornaz self-assigned this Jun 23, 2021
@zicklag
Copy link
Contributor Author

zicklag commented Jun 23, 2021

That's the best slution I can propose so far. If you want to try it yourself in this PR you can. But you don't have to ;-). I'll try to find time this week to do this change.

I think that sounds like a reasonable solution and much nicer. I might get to trying to do that, but I might not. Depends on what I get time for so if you beat me to it then I'll just let you do it. :)

@jcornaz
Copy link
Owner

jcornaz commented Jun 23, 2021

After some digging, I found that to achieve my vision I need a rust feature that isn't stable yet: https://doc.rust-lang.org/cargo/reference/unstable.html#weak-dependency-features

For the time being it looks like we must indeed introducedebug-2d feature. We don't need a debug-3d though, because heron doesn't have a 3d debug renderer.

I'll do some adjustements to your work and merge this PR. Thanks @zicklag ;-)

@jcornaz
Copy link
Owner

jcornaz commented Jun 23, 2021

@zicklag can you rebase your branch on my version of feature/separate-debug-features please?

If the build pass with these changes, it'll be ready for merge :-)

@zicklag
Copy link
Contributor Author

zicklag commented Jun 23, 2021

Sweet! Rebased and pushed.

@jcornaz jcornaz enabled auto-merge (squash) June 23, 2021 21:31
auto-merge was automatically disabled June 24, 2021 00:50

Head branch was pushed to by a user without write access

@zicklag zicklag force-pushed the feature/separate-debug-features branch from 2ed5307 to 379db4e Compare June 24, 2021 00:54
@zicklag
Copy link
Contributor Author

zicklag commented Jun 24, 2021

Just pushed a commit to fix the clippy lint name that broke CI.

@zicklag
Copy link
Contributor Author

zicklag commented Jun 24, 2021

Just pushed a change that further modifies the way features are handled so that none are default, 2d and 3d features must be enabled to pick one, but you can enable both ( aka. additive ) and it will just treat it as 3D and will compile and run fine. If you enable both, due to stable cargo limitations, it will still include the rapier2d dependencies, though so that would be definitely not recommended. We could otherwise add a compiler error when people do this to prevent accidents, though, which I think I prefer.

Not fully tested, but compiles and tests will run. Will test the demos tomorrow.

@jcornaz
Copy link
Owner

jcornaz commented Jun 24, 2021

Thanks @zicklag . I din't know cfg_aliases. I'll look into that :-)

@zicklag zicklag force-pushed the feature/separate-debug-features branch from 9af5b5f to cf9b837 Compare June 24, 2021 23:07
@zicklag
Copy link
Contributor Author

zicklag commented Jun 24, 2021

I had a bug in there that failed to enable debug rendering when the feature was supplied, just pushed a fix for that. All of the examples and tests should be working and passing now.

@zicklag zicklag mentioned this pull request Jun 24, 2021
@zicklag zicklag force-pushed the feature/separate-debug-features branch from cf9b837 to 4a00a35 Compare June 25, 2021 14:17
@zicklag
Copy link
Contributor Author

zicklag commented Jun 25, 2021

Pushed a commit that should fix CI's complaints.

Copy link
Owner

@jcornaz jcornaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Thank you very much @zicklag!

@jcornaz jcornaz merged commit 6e0ae98 into jcornaz:main Jun 25, 2021
jcornaz added a commit that referenced this pull request Jun 25, 2021
@zicklag zicklag deleted the feature/separate-debug-features branch June 25, 2021 19:19
@zicklag
Copy link
Contributor Author

zicklag commented Jun 25, 2021

Yay! Thank you for heron! It's doing a great job so far.

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

Successfully merging this pull request may close these issues.

None yet

2 participants