-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feature: rework location change event #4705
Conversation
} | ||
|
||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here.
|
||
@Override | ||
public void update(float delta) { | ||
for (EntityRef entity : process) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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.