Removed inner module from generated code. Implemented component update sending. #66
Conversation
send_component_update.
| match op { | ||
| WorkerOp::ReserveEntityIdsResponse(response) => { | ||
| if let StatusCode::Success(response_data) = response.status_code { |
There was a problem hiding this comment.
We could make this failure tolerant pretty easy 😉
There was a problem hiding this comment.
Could you clarify what you mean here?
There was a problem hiding this comment.
Commands can fail and you can effectively retry the command with a bit of book keeping
There was a problem hiding this comment.
Ah, good to know! I ended up removing the logic that depends on the request succeeding (i.e. it now unconditionally creates the entity without first reserving an ID), but I can still add the logic for repeating the request if you'd like 🙂
| @@ -358,7 +358,7 @@ impl ProtocolLoggingParameters { | |||
| CString::new(self.log_prefix.clone()).expect("Received 0 byte in supplied log prefix."); | |||
|
|
|||
| Worker_ProtocolLoggingParameters { | |||
| log_prefix: log_prefix_cstr.as_ptr(), | |||
| log_prefix: "protocol-log-".as_ptr() as _,//log_prefix_cstr.as_ptr(), | |||
There was a problem hiding this comment.
As we discussed offline - we need to change this implementation
There was a problem hiding this comment.
Yep. Will fix this properly later.
There was a problem hiding this comment.
I've addressed the safety issue here by having ProtocolLoggingParameters own the CString directly. In order to avoid leaking implementation details (i.e. that it's using CString under the hood), I've made all the fields private and added setters. I've also added a note in the doc comment about allowing us to add parameters without breaking changes (similar to CommandParameters), though if that's not appropriate here let me know.
I've also opted to use expect() on the result of the CString conversion instead of returning a Result<_, NulError>. After giving it more thought, I don't think it makes a ton of sense to expose that error directly. While it's technically possible for a Rust string to contain a nul byte in the middle, it's both incredibly unlikely to happen in practice, and it's likely indicative of an error in the user's code.
* Fix command line parameters so that workers are launched correctly. * Setup "rusty" layer and add load balancing setup.
|
With @davedissian's permission, I've gone ahead and finished up this PR. I've setup the example project to demonstrate component updates by adding a
The following gif also shows multiple entities moving in and out of a single worker's area of authority/intersted:
@jamiebrynes7 @davedissian I'm going to give the code another review pass myself, but it should be ready for you to re-review at this point. |
randomPoison
requested review from
randomPoison
and removed request for
randomPoison
| let component_update = update.get::<example::Rotate>().unwrap(); | ||
| let state = world.get_mut(&update.entity_id).unwrap(); | ||
| let rotate = state.rotate.as_mut().unwrap(); | ||
| rotate.merge(component_update.clone()); |
There was a problem hiding this comment.
Should the merge method take the update by reference rather than value?
There was a problem hiding this comment.
I'm fine with it taking the update by value. If merge were to take the update object by reference, then it would need to copy the individual fields in order to apply them to the component. Since it ultimately needs the component data by value, it makes more sense to take the update by value, that way the calling code can determine if the value needs to be cloned. This seems to be the more common convention in Rust (i.e. let the calling code decide whether or not the clone the data).
Still doesn't seem to quite work yet. Not sure why.