chore: simplify DSV's affected entity update#1619
Conversation
| var oldLooped = (boolean) loopDescriptor.getValue(entity); | ||
| if (oldLooped != isLooped) { | ||
| // Loop status change; add to affected entities | ||
| affectedEntities.add(entityVariable); |
There was a problem hiding this comment.
This was indirection. Put into a set, affect later. Seemingly unnecessary, since the invariant is that the loop status changed from previous, and therefore we need to execute the update even if we had already executed it on this entity before. (The set wouldn't save any work.)
There was a problem hiding this comment.
This is incorrect; the reason why entities are added to a set is becauses shadow variables can have different isLooped status, and the looped shadow variable is true if any of them are looped. Consider this situlation:
@ShadowVariable(supplierName="distanceSupplier")
Long distance;
@ShadowVariable(supplierName="arrivalTimeSupplier")
LocalDateTime arrivalTime;
@ShadowVariable(supplierName="serviceStartTimeSupplier")
LocalDateTime serviceStartTime;
@ShadowVariable(supplierName="departureTime")
LocalDateTime departureTime;
@PreviousElementShadowVariable(sourceVariableName="visits")
Visit previous;
List<Visit> visitGroup;
@ShadowSources("previous")
public Long distanceSupplier() {
if (previous == null) {
return 0L;
}
return previous.distance + 10L;
}
// the normal code for arrivalTime, serviceStartTime, departureTime goes hereIn this case, distance can never be looped, but serviceStartTime might be. So if serviceStartTime is looped, and is updated before distance, the distance update would incorrectly set the loop status of the entity to false.
|
|
||
| if (isLooped) { | ||
| if (oldValue != null) { | ||
| affectedEntities.add(entityVariable); |
There was a problem hiding this comment.
This is effectively dead code. The add method would check if the loop descriptor is not null and it would only have scheduled the update in that case. Since the update in this case already happened above, this code is no longer necessary.
| } else { | ||
| var newValue = shadowVariableReference.calculator().apply(entity); | ||
| if (!Objects.equals(oldValue, newValue)) { | ||
| affectedEntities.add(entityVariable); |
There was a problem hiding this comment.
Same code, same logic.
| changedVariableNotifier.afterVariableChanged().accept(variableDescriptor, entity); | ||
| } | ||
|
|
||
| private static final class AffectedEntities<Solution_> { |
There was a problem hiding this comment.
Because we no longer defer the updates, we no longer need this.
| changed.clear(); | ||
| } | ||
|
|
||
| private void updateLoopedStatusOfAffectedEntity(Object affectedEntity) { |
There was a problem hiding this comment.
Because we no longer defer the updates (see below), this is no longer necessary.
|
Christopher-Chianelli
left a comment
Christopher-Chianelli
left a comment
There was a problem hiding this comment.
This will not work; the isLooped parameter is true if a particular shadow variable is looped on the entity, whereas the ShadowVariableLooped shadow variable is true if any shadow variable is looped on the entity. For instance, if you have distance (which cannot be looped) and serviceStartTime (which can be looped), doing the update without checking the loop status of all variables on the entity can result into an entity being incorrectly marked as looped/not looped.
| var oldLooped = (boolean) loopDescriptor.getValue(entity); | ||
| if (oldLooped != isLooped) { | ||
| // Loop status change; add to affected entities | ||
| affectedEntities.add(entityVariable); |
There was a problem hiding this comment.
This is incorrect; the reason why entities are added to a set is becauses shadow variables can have different isLooped status, and the looped shadow variable is true if any of them are looped. Consider this situlation:
@ShadowVariable(supplierName="distanceSupplier")
Long distance;
@ShadowVariable(supplierName="arrivalTimeSupplier")
LocalDateTime arrivalTime;
@ShadowVariable(supplierName="serviceStartTimeSupplier")
LocalDateTime serviceStartTime;
@ShadowVariable(supplierName="departureTime")
LocalDateTime departureTime;
@PreviousElementShadowVariable(sourceVariableName="visits")
Visit previous;
List<Visit> visitGroup;
@ShadowSources("previous")
public Long distanceSupplier() {
if (previous == null) {
return 0L;
}
return previous.distance + 10L;
}
// the normal code for arrivalTime, serviceStartTime, departureTime goes hereIn this case, distance can never be looped, but serviceStartTime might be. So if serviceStartTime is looped, and is updated before distance, the distance update would incorrectly set the loop status of the entity to false.
2 participants
No description provided.