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

Mark ghost nodes as experimental and partially feature flag them #15961

Merged
merged 11 commits into from
Oct 16, 2024

Conversation

alice-i-cecile
Copy link
Member

Objective

As discussed in #15341, ghost nodes are a contentious and experimental feature. In the interest of enabling ecosystem experimentation, we've decided to keep them in Bevy 0.15.

That said, we don't use them internally, and don't expect third-party crates to support them. If the experimentation returns a negative result (they aren't very useful, an alternative design is preferred etc) they will be removed.

We should clearly communicate this status to users, and make sure that users don't use ghost nodes in their projects without a very clear understanding of what they're getting themselves into.

Solution

To make life easy for users (and Bevy), GhostNode and all associated helpers remain public and are always available.

However, actually constructing these requires enabling a feature flag that's clearly marked as experimental. To do so, I've added a meaningless private field.

When the feature flag is enabled, our constructs (new and default) can be used. I've added a new constructor, which should be preferred over Default::default as that can be readily deprecated, allowing us to prompt users to swap over to the much nicer GhostNode syntax once this is a unit struct again.

Full credit: this was mostly @cart's design: I'm just implementing it!

Testing

I've run the ghost_nodes example and it fails to compile without the feature flag. With the feature flag, it works fine :)

@alice-i-cecile alice-i-cecile added A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 16, 2024
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Oct 16, 2024
@alice-i-cecile alice-i-cecile added the D-Straightforward Simple bug fixes and API improvements, docs, test and examples label Oct 16, 2024
@alice-i-cecile
Copy link
Member Author

@villor can I get your review here?

@JMS55
Copy link
Contributor

JMS55 commented Oct 16, 2024

Small comment: You don't actually have to add an experimental folder. You can make an experimental module in the parent module, and then re-export the ghost node module's contents under it. This is what we did with other stuff.

Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@alice-i-cecile
Copy link
Member Author

I think that an experimental folder is a bit cleaner :)

Copy link
Contributor

@villor villor left a comment

Choose a reason for hiding this comment

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

I’m glad we are keeping the ghost nodes! What’s an experimental game engine without experimentation 🤓

That’s a neat trick make it non-constructable while not having to change any of the traversal code.

I have no strong opinions on the file structure. Especially when there is only one experimental feature.

Solution looks good to me, spookiness included 🎃

Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@viridia
Copy link
Contributor

viridia commented Oct 16, 2024

Yes, it's thematically appropriate that ghost nodes rely on phantom data. I can't wait to see the halloween-flavored release notes!

Copy link
Contributor

You added a new feature but didn't update the readme. Please run cargo run -p build-templated-pages -- update features to update it, and commit the file change.

@alice-i-cecile
Copy link
Member Author

Yes, it's thematically appropriate that ghost nodes rely on phantom data. I can't wait to see the halloween-flavored release notes!

I was tempted to just use a boring () internal field, but the spooky synergy was too much to resist :D

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

I think I have a cleaner solution that avoids the need for a constructor. Otherwise, I think this is a good idea.

alice-i-cecile and others added 2 commits October 16, 2024 17:11
Co-authored-by: Zachary Harrold <zac@harrold.com.au>
@alice-i-cecile
Copy link
Member Author

@bushrat011899 doesn't work :) GhostNode would need to be constructed as GhostNode() instead, which is not any better. The GhostNode symbol when it's a tuple strat is treated as a constructor function in the compiler errors.

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Sad my suggestion didn't pan out, I forgot that a tuple-struct can be used like a function. Looks good to go!

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 16, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 16, 2024
Merged via the queue into bevyengine:main with commit 76744bf Oct 16, 2024
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-UI Graphical user interfaces, styles, layouts, and widgets C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants