-
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
Changes from all commits
1e5c8d0
b22ebf8
a0776d9
32c636d
da23241
06a7931
424d5f7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,67 +2,95 @@ | |
// SPDX-License-Identifier: Apache-2.0 | ||
package org.terasology.engine.logic.location; | ||
|
||
import com.google.common.base.Preconditions; | ||
import org.joml.Quaternionf; | ||
import org.joml.Quaternionfc; | ||
import org.joml.Vector3f; | ||
import org.joml.Vector3fc; | ||
import org.terasology.engine.entitySystem.event.Event; | ||
|
||
/** | ||
* A <i>notification event</i> that the location and/or rotation of the associated entity has changed. | ||
* <p> | ||
* This is similar to a {@link org.terasology.engine.entitySystem.event.BeforeAfterEvent BeforeAfterEvent} but holds the | ||
* last and current values for both {@code position} and {@code rotation}. | ||
*/ | ||
public class LocationChangedEvent implements Event { | ||
public final LocationComponent component; | ||
public final Vector3fc oldPosition; | ||
public final Quaternionfc oldRotation; | ||
public final Vector3fc newPosition; | ||
public final Quaternionfc newRotation; | ||
protected Vector3f lastPosition = new Vector3f(); | ||
protected Quaternionf lastRotation = new Quaternionf(); | ||
protected Vector3f currentPosition = new Vector3f(); | ||
protected Quaternionf currentRotation = new Quaternionf(); | ||
|
||
public LocationChangedEvent(LocationComponent newLocation) { | ||
this(newLocation, newLocation.position, newLocation.rotation, newLocation.position, newLocation.rotation); | ||
/** | ||
* INTERNAL: default constructor for de-/serialization. | ||
*/ | ||
protected LocationChangedEvent() { | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Vector3f oPosition) { | ||
this(newLocation, oPosition, newLocation.rotation, newLocation.position, newLocation.rotation); | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Vector3f oPosition, Vector3f nPosition) { | ||
this(newLocation, oPosition, newLocation.rotation, nPosition, newLocation.rotation); | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Quaternionfc oRotation) { | ||
this(newLocation, newLocation.position, oRotation, newLocation.position, newLocation.rotation); | ||
} | ||
/** | ||
* Create a new <i>notification event</i> about a location and/or rotation change. | ||
* | ||
* @param lastPosition the previous position; must not be {@code null} | ||
* @param lastRotation the previous rotation; must not be {@code null} | ||
* @param currentPosition the current position; must not be {@code null} | ||
* @param currentRotation the current rotation; must not be {@code null} | ||
*/ | ||
public LocationChangedEvent(Vector3fc lastPosition, Quaternionfc lastRotation, | ||
Vector3fc currentPosition, Quaternionfc currentRotation) { | ||
Preconditions.checkNotNull(lastPosition); | ||
Preconditions.checkNotNull(lastRotation); | ||
Preconditions.checkNotNull(currentPosition); | ||
Preconditions.checkNotNull(currentRotation); | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Quaternionfc oRotation, Quaternionfc nRotation) { | ||
this(newLocation, newLocation.position, oRotation, newLocation.position, nRotation); | ||
this.lastPosition.set(lastPosition); | ||
this.lastRotation.set(lastRotation); | ||
this.currentPosition.set(currentPosition); | ||
this.currentRotation.set(currentRotation); | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Vector3f oPosition, Quaternionfc oRotation) { | ||
this(newLocation, oPosition, oRotation, newLocation.position, newLocation.rotation); | ||
public Vector3fc lastPosition() { | ||
return lastPosition; | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Vector3f oPosition, Quaternionfc oRotation, | ||
Vector3f nPosition) { | ||
this(newLocation, oPosition, oRotation, nPosition, newLocation.rotation); | ||
public Quaternionfc lastRotation() { | ||
return lastRotation; | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent newLocation, Vector3f oPosition, Quaternionfc oRotation, | ||
Quaternionfc nRotation) { | ||
this(newLocation, oPosition, oRotation, newLocation.position, nRotation); | ||
public Vector3fc currentPosition() { | ||
return currentPosition; | ||
} | ||
|
||
public LocationChangedEvent(LocationComponent nComponent, Vector3fc oPosition, Quaternionfc oRotation, | ||
Vector3fc nPosition, Quaternionfc nRotation) { | ||
oldPosition = new Vector3f(oPosition); | ||
oldRotation = new Quaternionf(oRotation); | ||
newPosition = new Vector3f(nPosition); | ||
newRotation = new Quaternionf(nRotation); | ||
component = nComponent; | ||
public Quaternionfc currentRotation() { | ||
return currentRotation; | ||
} | ||
|
||
public Vector3f vectorMoved(Vector3f dest) { | ||
return oldPosition != null ? newPosition.sub(oldPosition, dest) : dest.set(0, 0, 0); | ||
/** | ||
* Compute the vector that has to be added to get from {@link #lastPosition()} to {@link #currentPosition()} and | ||
* store the result in {@code dest}. | ||
* <p> | ||
* If V is the computed difference between {@code last} and {@code current}, then the following equation holds: | ||
* <p> | ||
* <code>last + V = current</code> | ||
* | ||
* @param dest will hold the result | ||
* @return {@code dest} | ||
*/ | ||
public Vector3f positionDelta(Vector3f dest) { | ||
return currentPosition.sub(lastPosition, dest); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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; | ||
/** | ||
* Compute the rotation that has to be applied to get from {@link #lastRotation()} to {@link #currentRotation()} and | ||
* store the result in {@code dest}. | ||
* <p> | ||
* If D is the computed difference between {@code last} and {@code current}, then the following equation holds: | ||
* <p> | ||
* <code>last * D = current</code> | ||
* | ||
* @param dest will hold the result | ||
* @return {@code dest} | ||
*/ | ||
public Quaternionf rotationDelta(Quaternionf dest) { | ||
return lastRotation.difference(currentRotation, dest); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,23 +2,45 @@ | |
// SPDX-License-Identifier: Apache-2.0 | ||
package org.terasology.engine.logic.location; | ||
|
||
import com.google.common.collect.Sets; | ||
import org.terasology.engine.entitySystem.entity.EntityRef; | ||
import org.terasology.engine.entitySystem.entity.lifecycleEvents.OnAddedComponent; | ||
import org.terasology.engine.entitySystem.entity.lifecycleEvents.OnChangedComponent; | ||
import org.terasology.engine.entitySystem.event.ReceiveEvent; | ||
import org.terasology.engine.entitySystem.systems.BaseComponentSystem; | ||
import org.terasology.engine.entitySystem.systems.RegisterMode; | ||
import org.terasology.engine.entitySystem.systems.RegisterSystem; | ||
import org.terasology.engine.entitySystem.systems.UpdateSubscriberSystem; | ||
|
||
import java.util.Set; | ||
|
||
@RegisterSystem(RegisterMode.AUTHORITY) | ||
public class LocationChangedSystem extends BaseComponentSystem { | ||
public class LocationChangedSystem extends BaseComponentSystem implements UpdateSubscriberSystem { | ||
private Set<EntityRef> process = Sets.newHashSet(); | ||
|
||
@ReceiveEvent(components = {LocationComponent.class}) | ||
public void locationChanged(OnAddedComponent event, EntityRef entity, LocationComponent lc) { | ||
lc.clearDirtyFlag(); | ||
} | ||
|
||
@ReceiveEvent(components = {LocationComponent.class}) | ||
public void locationChanged(OnChangedComponent event, EntityRef entity, LocationComponent lc) { | ||
if (lc.isDirty() && (!lc.position.equals(lc.lastPosition) || !lc.rotation.equals(lc.lastRotation))) { | ||
process.add(entity); | ||
} else { | ||
lc.clearDirtyFlag(); | ||
} | ||
} | ||
|
||
@ReceiveEvent(components = LocationComponent.class) | ||
public void onItemUpdate(OnChangedComponent event, EntityRef entity) { | ||
LocationComponent lc = entity.getComponent(LocationComponent.class); | ||
if (!lc.lastPosition.equals(lc.position) || !lc.lastRotation.equals(lc.rotation)) { | ||
entity.send(new LocationChangedEvent(lc, lc.lastPosition, lc.lastRotation)); | ||
lc.lastPosition.set(lc.position); | ||
lc.lastRotation.set(lc.rotation); | ||
@Override | ||
public void update(float delta) { | ||
for (EntityRef entity : process) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||
LocationComponent lc = entity.getComponent(LocationComponent.class); | ||
if (lc != null) { | ||
entity.send(new LocationChangedEvent(lc.lastPosition, lc.lastRotation, lc.position, lc.rotation)); | ||
lc.clearDirtyFlag(); | ||
} | ||
} | ||
process.clear(); | ||
} | ||
} |
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
andcurrent
works well here as naming. InBeforeAfterEvent
we useold
andnew
- was just wondering whether we should be consistent here 🤔