High-level dynamic query API #202
Conversation
|
Cool stuff! I am interested in implementing an API for accessing ECS data with minimal restrictions, which would help make scripting language APIs less prone to panics or other UB. Here are some examples: https://gist.github.com/kabergstrom/0629cb8db763081f38c56ab61b178a68 The goal is to be able to write code with looser restrictions than the existing APIs, while sacrificing performance in a few ways. Parallelism will not be possible, and there will be more runtime overhead. Borrowing semantics will be upheld at runtime. Queries must follow visibility rules when adding/removing components/entities during iteration, which is not free. The changes should enable writing familiar code that closely resembles what can be written in popular game engines where gameplay code is single-threaded-only. The key benefit here compared to other engines is that code can access the same data as the existing parallel APIs, meaning users can migrate code between the APIs depending on the evolving needs of their project. |
Sounds interesting, but frankly it's completely orthogonal to my use case. :) I need parallel access simultaneously to scripting access, and dynamic typing support at the query level. You can see https://github.com/sdleffler/hv-dev for a working example of this in action. |
I'm interested in supporting scripting languages better as a part of improving productivity of gameplay coding, so a dynamically typed API on top of the internals required to implement this will be very valuable. Could you elaborate on how this is orthogonal to your usecase? What do you mean with "parallel access simultaneously to scripting access"? |
Absolutely! In my case, the The big problem with this API for me is that it doesn't make extracting dynamic types (from scripting language values) into static type parameters (such as |
|
Thanks for the write-up!
I think you'll find running things in parallel opportunistically to be problematic due to ordering issues. If positions of bullets are updated in parallel, and scripts are able to interact with inputs to that step, how will scripts know which frame's data they are interacting with?
The API I propose will guarantee stable addresses for data within the scope, meaning it'd be OK to remove lifetime specifiers as long as the scope panics on exit if component refs are still active (i.e. someone stores a Ref in a persistent object somewhere). Since Lua scripts are not supposed to hold onto component refs persistently, this seems like an OK compromise. I think it'd be reasonable to build an API similar to what you have in this PR on top of the internals of the API I'm proposing. It'll allow super-fine-grained borrow checking and no restrictions on add/remove of entities/components. Queries would not need to be scoped or require a closure in Lua. I believe our goals are aligned, in that we both want improved productivity for gameplay programming. I am interested in enabling productive gameplay coding in both Rust and Lua, while still making it possible to run other code that can access ECS data in parallel. |
Ordering is well-defined with what I'm working on due to dependency constraints between systems. For example, the bullets I'm talking about here are objects which have an extremely limited interaction with scripting; they can be spawned and scripting can define patterns and some very limited behavior, but once spawned, their behavior w.r.t. scripting is limited to callbacks on collision (which may or may not be collected and deferred.) So for example, systems might proceed with entering a section where scripting is given priority to call
This is how
It's not impossible to modify the
Definitely! :D |
|
I think I'm starting to see how this proposed API would be so helpful - a dynamic query could immediately run the query, collect all results, and return them, without keeping any actual borrow on the world, since it would basically only be a filtered |
Since the API wraps a &mut World, it gets to decide how to make access to the data stored in World safe, and can ignore archetype borrowing. Instead, the API would have a more granular borrow checking mechanism internally for guarding access to memory. The API must only ensure the functions it exposes are sound during the scope, and doesn't have to ensure the regular World APIs are safe to call. |
What about the lifetime of the wrapper type itself? Since it would wrap |
Yes, there would be reference+borrow counters internally. In the Drop impl, if there are active references, it panics which causes it to never swap back ownership. This ensures that the. World cannot escape the scope while there are active references, even if the user mem::forgets the scope (since that would just leak the world and leave the user with an empty world). |
There was a problem hiding this comment.
Made an initial pass. Please add:
- a minimal example that exercises the new capabilities this introduces, e.g. by manipulating a world through an FFI-safe interface.
- tests that verify proper borrow-check panics
Also, did you consider a more direct AST-based approach in place of relying heavily on dynamic dispatch? What were the tradeoffs that led this way?
| empty @ None => empty | ||
| .insert(Box::new(value)) | ||
| .downcast_mut() | ||
| .expect("dynamic fetch invalidated!"), |
There was a problem hiding this comment.
This can't possibly fail, so a message isn't useful.
| .expect("dynamic fetch invalidated!"), | |
| .unwrap(), |
| /// Create a new `DynamicQuery`. Use this to compose dynamic queries in a similar way to how you | ||
| /// would use tuples to compose [`Query`]s. |
| } | ||
|
|
||
| /// Lift a static [`Query`] into a `DynamicQuery`. | ||
| pub fn lift<Q: Query + Send + Sync>() -> Self |
There was a problem hiding this comment.
Send + Sync shouldn't be needed here, since values of type Q are never constructed. Instead, StaticFetch should have unsafe impl Send + Sync. Also, it's kind of weird to mix inline bounds with a where clause.
| /// Lift a static [`Query`] into a `DynamicQuery`. | ||
| pub fn lift<Q: Query + Send + Sync>() -> Self | ||
| where | ||
| Q: 'static, |
| }; | ||
|
|
||
| #[derive(Default)] | ||
| pub struct DynamicFetch { |
| } | ||
| } | ||
|
|
||
| pub trait ErasedFetch<'a>: Send + Sync { |
There was a problem hiding this comment.
Does the lifetime 'a here serve a purpose? For Fetch it's used so that Item can have an appropriately bounded lifetime, but here we're dynamically checking those lifetimes anyway.
| /// A dynamic query; the dynamic equivalent of the [`Query`] trait. Used with | ||
| /// [`World::dynamic_query`] and [`World::dynamic_query_one`]. |
There was a problem hiding this comment.
We should elaborate on what exactly "dynamic equivalent" means, e.g. by stating that the query has dynamic constraints and as a result yields dynamically-typed items. There should also be discussion of the lifetime of DynamicItem, and detailed discussion of the situations in which all this is needed.
| /// As with [`QueryBorrow`](crate::QueryBorrow), borrows are not released until this object is | ||
| /// dropped. When this object is dropped, all [`DynamicItem`]s and [`DynamicComponent`]s created | ||
| /// from it are invalidated (and if any are still borrowed, a panic will occur.) | ||
| pub struct DynamicQueryBorrow<'w> { |
There was a problem hiding this comment.
Does this need to be a separate type from DynamicQueryIter? We do that for static queries so that the dynamic borrow-check is guaranteed to outlive the static borrows yielded by QueryIter, but DynamicQueryIter does not yield static borrows.
| } | ||
|
|
||
| /// Iterator over the set of entities which satisfy some [`DynamicQuery`]. | ||
| pub struct DynamicQueryIter<'q> { |
There was a problem hiding this comment.
My understanding is that the 'static lifetime of DynamicItem is motivated by the requirement to safely pass it to code that is not lifetime-checked (e.g. foreign code). I therefore infer that DynamicQueryIter cannot be passed in that fashion. What impact does this have on the objective of allowing foreign code to query the ECS?
| @@ -72,6 +73,10 @@ mod world; | |||
| pub use archetype::Archetype; | |||
| pub use batch::{BatchIncomplete, BatchWriter, ColumnBatch, ColumnBatchBuilder, ColumnBatchType}; | |||
| pub use bundle::{Bundle, DynamicBundle, MissingComponent}; | |||
| pub use dynamic_query::{ | |||
| DynamicComponent, DynamicItem, DynamicQuery, DynamicQueryBorrow, DynamicQueryIter, DynamicWith, | |||
| DynamicWithout, Ref as DynamicItemRef, RefMut as DynamicItemRefMut, | |||
There was a problem hiding this comment.
I prefer not to use as reexports as it makes the rustdocs confusing. In particular, function signatures are not rewritten to use the reexported name, at least last time I checked. Unless that's changed, we should use the desired public name everywhere.
|
Thank you for the review! I'm going to try to go through it today. If I understand exactly what you mean by "a more direct AST-based approach", I went with dynamic dispatch because it ensures that new query combinators didn't have to be added manually to the dynamic query machinery, and allows use of external query combinators without any special machinery. To elaborate on that, I thought that since |
Critically, it's not clear to what extent clever custom However, #92 establishes weak precedent for getting cheeky here. Maybe there are other cases? Are they worth pursuing, given that we're not currently supporting external extensions here? Another relevant factor: how do you envision scripting language constructing/executing queries, if at all? Natively |
This // Create a Lua context.
let lua = Lua::new();
// Load some builtin `hv` types into the Lua context in a global `hv` table (this is going to
// change; I'd like a better way to do this)
let hv = hv::lua::types(&lua)?;
// Create userdata type objects for the `I32Component` defined above as well as a similarly defined
// `BoolComponent` (exercise left to the reader)
let i32_ty = lua.create_userdata_type::<I32Component>()?;
let bool_ty = lua.create_userdata_type::<BoolComponent>()?;
// To share an ECS world between Lua and Rust, we'll need to wrap it in an `Arc<AtomicRefCell<_>>`.
// Heavy provides other potentially more efficient ways to do this sharing but this is sufficient
// for this example.
let world = Arc::new(AtomicRefCell::new(World::new()));
// Clone the world so that it doesn't become owned by Lua. We still want a copy!
let world_clone = world.clone();
// `chunk` macro allows for in-line Lua definitions w/ quasiquoting for injecting values from Rust.
let chunk = chunk! {
// Drag in the `hv` table we created above, and also the `I32Component` and `BoolComponent` types,
// presumptuously calling them `I32` and `Bool` just because they're wrappers around the fact we
// can't just slap a primitive in there and call it a day.
local hv = $hv
local Query = hv.ecs.Query
local I32, Bool = $i32_ty, $bool_ty
local world = $world_clone
// Spawn an entity, dynamically adding components to it taken from userdata! Works with copy,
// clone, *and* non-clone types (non-clone types will be moved out of the userdata and the userdata
// object marked as destructed)
local entity = world:spawn { I32.new(5), Bool.new(true) }
// Dynamic query functionality, using our fork's `hecs::DynamicQuery`.
local query = Query.new { Query.write(I32), Query.read(Bool) }
// Querying takes a closure in order to enforce scope - the queryitem will panic if used outside that
// scope.
world:query_one(query, entity, function(item)
// Querying allows us to access components of our item as userdata objects through the same interface
// we defined above!
assert(item:take(Bool).value == true)
local i = item:take(I32)
assert(i.value == 5)
i.value = 6
assert(i.value == 6)
end)
// Return the entity we spawned back to Rust so we can examine it there.
return entity
};Queries w/ multiple entity look identical in my current implementation, where the only difference is As for when you'd actually want to do it, I envision it for prototyping or even debugging, for just quickly extracting a bunch of entity data. |
|
That example doesn't seem to contain
Can you elaborate on when lifting complex static queries in one go is helpful here? My intuition is that if you're writing the whole query in Rust to begin with, discarding type information doesn't do you much good. |
Yes; for reference, under the hood, impl<T: ecs::Component + UserData> ComponentType for Type<T> {
fn type_id(&self) -> TypeId {
TypeId::of::<T>()
}
fn read(&self) -> ecs::DynamicQuery {
ecs::DynamicQuery::lift::<&T>()
}
fn write(&self) -> ecs::DynamicQuery {
ecs::DynamicQuery::lift::<&mut T>()
}
// ... There is significantly more to this impl, but it isn't relevant here.
}They then call
Sure; if you want to create a query which is dynamically usable and iterates over a bundle, then you can export a What would you prefer over |
|
I'm just looking for strong motivation for or against the trait object approach vs something more AST-y, where If all other factors are a wash, then we might as well have |
So, I don't have a lot of knowledge of HECS specifics yet which leaves my opinion of this solution fairly undefined. In general though, I wanted to run through my initial thoughts and see where they fit into the two approaches being discussed. Please correct any misconceptions I may have when I outline this because, as stated, still getting up to speed on the code base and so far I'm mostly just working on integration layer items. The $0.10 cent outline:
The basic thinking with #1 is that at the bottom most level, the only thing needed from what I could see to filter the archetypes and identify columns is the TypeId and read or write access for borrow tracking. The query can also likely be plumbed out to support a 'prepared' version to get a performance benefit like other API's. For #2 & #3 using slices rather than iterators is meant to allow the solution to be used in more use-cases without performance degradation. Specifically, passing one entity over to C/C++ at a time will incur major overhead because the loops won't be unrolled, accessors can't be inlined, etc etc. So, leaving the raw access at this level means that compiled languages can optimize properly. At the same time, a fairly light wrapper over that adds the per entity iteration back in so it acts pretty much like the original API's much as this PR would be exposing. Overall, I believe this would work and leave the solution open to more use cases without making major changes to hecs itself. If I'm completely out in left field here, let me know, this was my first pass thinking before looking at this PR. |
|
You can actually accomplish that particular pattern already through the public API. See |
That's very close but it takes a 'T: Component' and returns a typed column which I won't be able to use. Since I'll only likely have a TypeId and a Layout and won't be able to specialize to the component types, the intent was to add a 'get_dynamic(id: TypeId) -> &[u8]' or something like that, and yes, a mut version also. Totally unsafe but since it is going over the FFI boundary, nothing is going to make it safe. The only thing that is actually a bit worrisome to me is TypeId itself. The std libs bury the details on purpose and yell loudly about "it can change at anytime", so the fact that I'm going to have to transmute those to/from u64 is annoying. I won't be storing them or anything but they could decide to make them u128 or go back to u32 for all we know. So, I'll have to watch for changes each release. :( Hopefully I'll convert most of the C++ to Rust over time but till that is complete it will remain a worry point. Update: I went ahead and implemented a "get_dynamic_slice(&TypeInfo) -> Option<&[u8]>" and mut variation. I think this is all I need to accomplish my goals with the FFI and if so, I'll push a PR for it. |
Given that your actual types are static, you can type-erase
There are, in fact, plans to do exactly that. For portability between rust versions, and arguably soundness in general, you shouldn't
I think this is unsound because it could construct a reference to uninitialized padding bytes. Returning a raw pointer should be safer. |
I think we're potentially getting some wires crossed here. The issue is I don't have Rust types at all, these components will be defined in C/C++ until ported over to Rust. So, I'm going to force feed them into hecs by supplying TypeInfo's from the data provided through the FFI. This is only appropriate for things which are fully opaque to the Rust side and are repr(C), meaning that only C will access the components in any manner and any Rust interaction will be via FFI to API's on that side. I'll port as fast as possible but it will take a while as I unwind a fairly large amount of code over there in between other goals.
Yup, but as mentioned above, the components in question won't have any valid type to get a TypeId from on the Rust side, just a layout struct and a drop function. This is not "all" of the components, just a number of them which are too large to port in any reasonable amount of time. I have looked into how the TypeId is generated and it is basically already how I generate the unique identifiers in the C code, barring any actual hash collisions, it will work as a stop gap. Totally evil though.
I'm not sure I see how it would be unsound, totally absolutely without question very unsafe. Underlying the archetype grow_exact is basically the creation of a strided array, it is identical to the underlying code I'm replacing on the C side of the FFI so plugging the data straight through is perfect for that. So long as the C side only accesses repr(C) elements and treats any repr(rust) as opaque, there is no questionable access happening. Maybe I'm missing some finer point here? Having said this, I am a little concerned with one item; the way the strided array is allocated, hopefully there is a subtle solution I've missed. Basically if you track things back to TypeInfo, the layouts are created directly from the parameterized type and not modified. In grow_exact, that layout is used to create a layout for the entire array container, size simply multiplied by the number of elements. The concern is that at no point could I find it calling "Layout::pad_to_align" or "Layout::array" to guarantee the memory is allocated properly for storage of an array of T's. In "most" cases it probably would not be a problem but I suspect any sufficiently goofy component like an _m128+bool component would end up with insufficient allocated space since it would not add the 15 bytes per component needed to satisfy alignment within the array. Is this correct or did I miss something? |
|
@All8Up if I understand correctly, you could benefit from a few mechanisms that I use on the regular with regards to dynamically representing types. Also, "force-feeding" C data into hecs and providing custom |
I don't believe I can use that solution unfortunately. The point of the FFI for my needs is to get rid of an old crusty piece of C++ that manages archetypes, which is exactly what hecs provides. I'm basically converting to Rust from the inner storage engine outwards. The reason I looked at hecs in the first place is that at the core of the archetype, other than the language in question, it works pretty much identically to the core of the crusty C++ I wrote about 5ish years ago. So, it seems like a pretty brain dead process, I just have to jump through a few hoops for a bit till I port more code to Rust. Also, it was already difficult to sell folks on using Rust in the first place, adding unstable would likely not go over well and even as the boss I won't mandate it.
No doubt, it is "totally, without question, evil and unsafe".. :) But, on the other hand, when 50,000 entities is a "small test" for our simulation work, I can't really afford any added overhead. The reason to do away with the C implementation is the simple fact that due to borrow checking in Rust, the thread safety we require is part of the compiler of course. The biggest ongoing issue is that folks write unsafe components/systems and I have to go spend a week debugging something as silly as "you didn't use an atomic".. When your minimum machine starts with 16 cores and suggested is 32 or 64, these issues are significant.
Ah, but I have two requirements. One is very appropriate here which is that the bt (behavior tree) compiler is effectively a script language and after compilation (which I wrote in Rust originally, so that part is already good to go) I have to rebuild the execution pipeline and the per system component access on the fly during hot reloads. The bt's know all the components they can access but based on what a user puts into the nodes, the system(s) have to dynamically query hecs. So, this PR is very much of interest. BT's are just one of several items where the system component access is not known at compile time, so your work here is very much of interest. I probably should not have posted the initial comment here but in the other PR, I realized the two areas of my needs are too divergent and part of it does not fit this PR. Sorry about that.. |
|
Ah, I see! I didn't realize you had two different things you needed out of this rough patch of feature-space. Makes sense! :) |
This adds a (comparatively high level) API for performing dynamic queries, which is more flexible than (but much less performant than) the approach outlined in #196. The API is based on experiments exposing hecs to Lua as can be found in the repository at https://github.com/sdleffler/hv-dev. Much of it is based heavily on internal use of dynamic types and "trait-object-ifying" the
Fetchtrait, which has a lot of performance consequences. Performance probably isn't much of a concern for anyone who'd want to use this API to integrate with, say, a scripting language... but if necessary there are points which can be optimized, by pooling allocated memory for dynamic fetches and such.