aevyrie / big_space

Floating origin plugin for spaces larger than the universe
Apache License 2.0
168 stars 22 forks source link

third party bevy UI goes missing after adding plugin / IgnoreFloatingOrigin but no GridCell<P> #19

Closed johnny-smitherson closed 5 months ago

johnny-smitherson commented 6 months ago

Any existing or external bevy UI nodes dissapear from the screen after the plugin is in use, and if the camera leaves the starting cell.

Example third-party UI: https://github.com/laundmo/bevy_screen_diagnostics

After some unsuccessful debugging and diving back into the examples one finds that all UI Nodes must have IgnoreFloatingOrigin.

The workaround after that is simple (for UI Nodes at least) -

fn main() {
      ...
    .add_systems(Update, ignore_all_ui_nodes_from_floating_origin)
      ...
}

fn ignore_all_ui_nodes_from_floating_origin(mut commands: Commands, q: Query<(Entity, Option<&IgnoreFloatingOrigin>), With<Node>>) {
    for (node_ent, node_ignored) in q.iter() {
        if node_ignored.is_none() {
            commands.entity(node_ent).insert(IgnoreFloatingOrigin);
        }
    }
}

The snippet above fixes any third-party UI, or UI that was there before adding the plugin. But, there might always be some more entities entities that have Transform&Global Transform, but don't have (and won't have) a GridCell<P> when they spawn: custom directional audio, custom 2D stuff, importing serialized scenes from Blender, etc.

So now the workaround now is

fn ignore_all_non_grid_from_floating_origin(mut commands: Commands, q: Query<(Entity, Option<&big_space::IgnoreFloatingOrigin>, Option<&big_space::GridCell<i64>>)>) {
    for (node_ent, node_ignored, node_gridcell) in q.iter() {
        if node_ignored.is_none() && node_gridcell.is_none() {
            commands.entity(node_ent).insert(big_space::IgnoreFloatingOrigin);
        }
    }
}

But now, there's a chance we don't add the IgnoreFloatingOrigin flag soon enough, so we have to schedule this before the inaccessible system sets and systems that do the propagaion - so we'll have a hard time scheduling the workaround in the same PostStartup & PostUpdate schedules if we happen to need that.

I think all these complications would go away if the plugin didn't touch any of the Transforms/GlobalTransforms for entities that don't have GridCells.

But I have some questions:

Or am I getting this backwards and the plugin will insert GridCell<P> for me everywhere it finds Transform and I should only be ever reading from the GlobalTransform and should rewrite/fork/vendor every crate that doesn't?

aevyrie commented 6 months ago
  • could we make the anonymous system set public and externally accessible, so we can use before on it?

The floating origin plugin is already in bevy's TransformPropagation set: https://github.com/aevyrie/big_space/blob/cf926e7edb1a3246d9b907f317ee2400617a20cd/src/lib.rs#L182

All systems in the plugin are inside this set, so ordering should be simple. This was a very intentional choice to remain compatible with the ecosystem. Many plugins already use TransformPropagation for ordering, which means they will work with bevy or big_space's transform propagation systems.

  • if entity does not have GridCell<P>, could it be ignored from floating point origin by default? Or is there some use-case where it makes sense for the plugin to modify the GlobalTransform without actually having a GridCell<P> ?
  • is there any situation in which one would want not to have IgnoreFloatingOrigin on 2D UI nodes?

It's a bit late here, so I'm just going to describe how things work, and we can go from there. :)

The recent addition of reference frames complicated this. Previously, only entities with GridCell would participate in the floating origin, and children that had a Transform but no GridCell would have their transform propagated. Reference frames made everything a bit more consistent and work with arbitrary entity hierarchy depth:

This is great because it allows arbitrary nesting of reference frames with high precision, however things get weird at the root of the tree, because there is no parent. It seems reasonable to expect that an entity with a GridCell and no parent would be implicitly added to a root reference frame - this was true before reference frames were made explicit! To remain consistent with the rest of the hierarchy, entities at the root with no parent but with a Transform should behave like they are in the root reference frame.

This is why the IgnoreFloatingOrigin component was added. Now that everything in the root behaves like it is, well, in the root reference frame, then things that use Transform but don't exist in 3d space, like UI, will break. If anything, I think this is an agument for why bevy_ui should not be using Transform unless it wants to position that entity in world space.

johnny-smitherson commented 6 months ago

It's a bit late here

It's lunch here but the kebab shops are closed please excuse hungry tone

ordering should be simple

Yes it works, thanks, maybe we put reminder in the docs page for the plugin https://github.com/aevyrie/big_space/pull/20

bevy_ui should not be using Transform unless it wants to position that entity in world space.

Yes, I see how that design decision complicates out problem here. But it's not just bevy_ui. It'll be all plugins that use the Bevy ECS independent of what happens outside them. This is (at least in my experience) pretty inevitable once you start to like the ECS.

For example, I have this custom plugin for directional audio. I spawn a large set of "audio" entities invisible to the outside world, parent them to nothing, give them SpatialBundle just to have a Vec3 to write and read from, do some Vec3 computations in a different coordinate space than the normal world, trace some rays, filter out the 16 sounds you're going to hear this frame. Same story for VFX plugin using this, NPC pathfinder crate.

All data that flows into these is already converted from GlobalTransform into some kind of local coordinate system (thanks for the free precision), but the current implementation will smash their transforms, so I would have to go into all these and add the Ignore flag, which also means adding a dependency on this crate (or hunt for them in the main game plugin like the above).

Because of the way Bevy works without this plugin, this kind of code is easiest to write when you assume that, IFF you have your entity with no parent, then Transform = GlobalTransform = whatever you put in it just now. In these kind of situations, it's comfortable not to use GlobalTransform because you either have a 1-frame latency in the value, or need to schedule reads at the very end of PostUpdate and writes before that - so you just assume the above and carry on.


Why not make the RootReferenceFrame frame explicit?

Users would create their own "universe" entity that is the root, and all descendants must have GridCell, else panic. Each RootReferenceFrame must have a single FloatingOrigin descendant, else panic. If a grid cell is not a descendant of a root reference frame, panic. If the root frame has a parent of its own, panic. We could even panic if the root entity that the user gave us has a Transform or Gridcell that isn't zero, or if it has those components at all, or anything else you want to assume in the implementation.

The Ignore flag can still function on descendants of the root, I can't imagine any use-case (since I can control what I parent to the universe) but maybe you can. If not, removing Ignore would simplify your internals.

And so, the ecosystem outside this plugin that may be running in the same App will not be affected in any way.

Maybe this "explicit RootReferenceFrame" could be an optional switch in the Plugin definition to avoid breaking change. But the RootReferenceFrame would need to change from resource to component, which is also a breaking change.

This could also help with the other ticket #17 - just spawn 2 universes, each with its own FloatingOrigin descendant.

And with #21 - if no entity with RootReferenceFrame, do nothing. If no FloatingOrigin as descendant of the root, assume it's zero.

aevyrie commented 6 months ago

Yes, I see how that design decision complicates out problem here. But it's not just bevy_ui. It'll be all plugins that use the Bevy ECS independent of what happens outside them. This is (at least in my experience) pretty inevitable once you start to like the ECS.

The issue is that if you are using Transform, you need to be using it with the same semantics everywhere. I.e. "this entity is positioned relative to its parent in cartesian world space". The reason the plugin behaves the way it does is that if you are using another plugin with the same semantics (that's the point of components!), it will all continue to work as normal. This plugin will see those entities, and treat the spatial components like... spatial components. If you are using another 3rd party plugin that spawns a building with a Transform at (200, 200, 200), then this plugin will continue to respect that. As you fly away and change grid cells, it will update its GlobalTransform so it renders at (200, 200, 200) in the root reference frame. If it did not do this, then it would appear like the building was teleporting toward the camera as you moved away.

Another way to think about it is the transform propagation system owns Transform -> GlobalTransform. If you want different behavior, you should be using a different transform component. For the audio example, because you are giving them a spatial bundle, you are saying you want them to behave like a spatial bundle!

I'm not saying "you're holding it wrong", I'm saying "If you tell me something is a spatial entity, I will treat it like a spatial entity".


I did actually consider making reference frames always explicit, but thought that it would make it harder to interoperate with the ecosystem. However, I think you've convinced me to switch.

It has the additional benefit of simplifying things

The hierarchy would look like (ignoring globaltransform):

Entity (Transform, AudioStuff) // Managed by bevy_transform
Entity (Transform, Handle<Mesh>) // Managed by bevy_transform

// everything past this point is valid and managed by big_space
Entity (RootReferenceFrame) // has no transform, bevy_transform ignores this
    Entity (Transform, Handle<Mesh>) // low precision mesh
    Entity (GridCell, Transform, Handle<Mesh>) // high precision mesh
    Entity (ReferenceFrame, GridCell, Transform) // child reference frame
        Entity (Transform, Handle<Mesh>) // low precision mesh, in parent frame
        Entity (GridCell, Transform, Handle<Mesh>) // high precision mesh, in parent frame

And with https://github.com/aevyrie/big_space/issues/21 - if no entity with RootReferenceFrame, do nothing. If no FloatingOrigin as descendant of the root, assume it's zero.

I think the requirement changes to "each root reference frame subtree can only have one floating origin". Instead of needing to validate this for every entity with FloatingOrigin, we could instead change the API to make it impossible to express an invalid hierarchy. I.e., remove the FloatingOrigin component entirely, and instead have a RootReferenceFrame{ origin: Entity } component. That entity would need to be in the hierarchy, so I guess you only need to do a single validation check for each universe - iterate through the parents of this entity, and ensure it leads you back to the root reference frame that points to the entity. If it does not, then error and skip updating that universe until the entity is valid.

johnny-smitherson commented 6 months ago

RootReferenceFrame{ origin: Entity }

Sure - that complicates setup a bit (you need to create the origin, then create the root, then parent origin to root, then insert the RootReferenceFrame{ origin: Entity }) - feels a bit backwards.

Since propagate_recursive() needs to get into all the children's children every frame anyway, could it be rewritten in terms of iter descendants? This would remove the need for the plugin to do recursivity or unsafe, and you can query the parent on each of the children returned to get the graph. Then, we could use that info to do a topological sort and add some concurrency to the algo to improve perf (all children of same distance to root can be propagated in parallel).

While we have all the descendants, we could also count the FloatingOrigins and explode if there is more than 1.So we wouldn't need to run iter_ancestors every turn to see if the FloatingOrigin is misplaced.

aevyrie commented 6 months ago

Sure - that complicates setup a bit (you need to create the origin, then create the root, then parent origin to root, then insert the RootReferenceFrame{ origin: Entity }) - feels a bit backwards.

Fair, it could just be Optional, then you can set up the world, place the origin, and set it. I'm shying away from the FloatingOrigin component because it can lead to situations where a spawn or insert command hasn't run, so there is now a chance of a frame delay or crash if you aren't careful with ordering. Setting a value in the root component would be immediate and it should completely prevent this specific edge case.

Since propagate_recursive() needs to get into all the children's children every frame anyway

That's not quite how it works. Propagation runs up and down the tree, away from the floating origin. iter_descendents won't work because transforms are relative to their immediate parent, you need to compute them in order. The unsafe there is an optimization - it's a feature not a bug - it allows you to update branches of the tree in parallel because the shape of the tree prevents aliasing.

While we have all the descendants, we could also count the FloatingOrigins and explode if there is more than 1.So we wouldn't need to run iter_ancestors every turn to see if the FloatingOrigin is misplaced.

That would be much more expensive. Iter_descendents is recursive, because it has to unpack the entire hierarchy into a flat list. iter_ancestors is linear with the depth of the tree. Even if you wanted to simulate the entire universe, you don't need more than a dozen levels of depth; that check would take somewhere on the order of microseconds. You also only need to do this check once per universe per frame to ensure the floating origin is valid before it starts propagation.

johnny-smitherson commented 6 months ago

Ah yes I see now iter_descendants is linear and single-threaded with a deque, even through I expect collecting these (child entity, parent entity) tuples into a vector will take much less than the actual matrix multiplications that will be going on.

https://docs.rs/bevy_hierarchy/0.13.2/src/bevy_hierarchy/query_extension.rs.html#111-116

I like the optional, would let me not have the origin for a few frames for #21

aevyrie commented 6 months ago

I've been working on a branch that implements this (#22). Turned into a pretty massive overhaul in some ways. It now seems to be largely working. Entities at the root of the hierarchy use bevy's built-in transform propagation system, and everything within a big space hierarchy has its GlobalTransform computed relative to the floating origin within that hierarchy.

I've ended up keeping the FloatingOrigin component, but also adding the optional we discussed. The optional at the root is the source of truth, and the floating origin component is essentially just a helper to make it easier to work with - it would be a bit annoying to keep track of otherwise. Having zero or more than one floating origin should now just result in a logged error, and that optional being set to None. This will result in propagation pausing for that "universe" (I'm calling them BigSpaces now, for obvious reasons 🙂).

johnny-smitherson commented 6 months ago

BigSpaceBundle vs. BigSpatialBundle tickles my dyslexia but otherwise works great.

pretty massive overhaul

As user the change list is very minimal, Res<RootReferenceFrame> --> Query<&ReferenceFrame> on parent + the various renames.

https://gist.github.com/johnny-smitherson/2a9427668998187532c04af93a5d84c3

Thanks for the help!

aevyrie commented 6 months ago

I should reconsider the BigSpace naming in that case. It's cute, but perhaps misleading and confusing.

As user the change list is very minimal,

Good to see!