feature: rework location change event #4705
Conversation
github-actions
bot
added
the
Type: Improvement
| } | ||
|
|
||
| public LocationChangedEvent(LocationComponent newLocation, Quaternionfc oRotation, Quaternionfc nRotation) { | ||
| this(newLocation, newLocation.position, oRotation, newLocation.position, nRotation); | ||
| public Vector3fc lastPosition() { |
There was a problem hiding this comment.
I think last and current works well here as naming. In BeforeAfterEvent we use old and new - was just wondering whether we should be consistent here 🤔
|
|
||
| public Vector3f vectorMoved(Vector3f dest) { | ||
| return oldPosition != null ? newPosition.sub(oldPosition, dest) : dest.set(0, 0, 0); | ||
| return currentPosition.sub(lastPosition, dest); |
There was a problem hiding this comment.
This changes the semantic for the very first change event, doesn't it? Previously we return the zero-vector, now we return the current position.
Where is this used, and does it make a difference?
There was a problem hiding this comment.
we don't actually use it in any capacity and its such a simple wrapper. I think I would probably drop it.
There was a problem hiding this comment.
We can keep it, that's not the problem (and my intention with the question was not to get rid of it).
In particular, I think that
locationChange.vectorMoved(dest);
// or
locationChange.positionDelta(dest);is way better to read than doing the computation on receiver side
locationChange.currentPosition().sub(locationChange.lastPosition(), dest);| } | ||
|
|
||
| public float distanceMoved() { | ||
| return oldPosition != null ? newPosition.distance(oldPosition) : 0.0F; | ||
| return currentPosition.distance(lastPosition); |
|
|
||
| @Override | ||
| public void update(float delta) { | ||
| for (EntityRef entity : process) { |
There was a problem hiding this comment.
Would be interesting to collect some metrics here on how many entities we typically process per tick. Could easily be added in a follow up PR, I guess.
|
|
||
| @ReceiveEvent(components = {LocationComponent.class}) | ||
| public void onItemUpdate(OnChangedComponent event, EntityRef entity) { | ||
| public void locationChanged(OnAddedComponent event, EntityRef entity) { |
There was a problem hiding this comment.
This can be rewritten to directly get the LocationComponent as argument.
@ReceiveEvent
public void locationChanged(OnAddedComponent event, EntityRef entity, LocationComponent lc) {
lc.isDirty = false;
}| LocationComponent lc = entity.getComponent(LocationComponent.class); | ||
| if (lc != null) { | ||
| entity.send(new LocationChangedEvent(lc.lastPosition, lc.lastRotation, lc.position, lc.rotation)); | ||
| lc.isDirty = false; |
There was a problem hiding this comment.
Instead of setting the dirty flag to false directly, should we rather add a package-private method clean() which does this? Trying to avoid accidentally setting it to true for some reason on a child location that has a parent...
…re/change-location-component-event
skaldarnar
added
the
Category: Doc
I changed the location change event to be handled on the next frame. if you change the location position then I mark the last position and prevent the last position/rotation from getting changed. only on the next update do I unlatch the flag so the location is relative to the last frame instead of whenever the component is changed then saved. this is more consistent since you could do something like this and have the component for every change.