Require Order ID in prepare msg and score automatically #16
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We now also require that the prepare message for a DS contains the order that is to be delivered.
When a DS is prepared, also store the order ID that the DS was prepared for. This can later be used to check whether the correct product was delivered.
If the DS is in state PROCESSED, directly assert a product-delivered fact with the new confirmed slot set to FALSE. Previously, the fact was only created when a referee would enter a delivery into the refbox. As we already know which order was delivered, directly assert the fact to keep track of the order. This will require further changes in the processing of the order.
We already have the product-delivered field (which is now set when the DS is PROCESSED). If we receive a SetOrderDelivered message, just change the order from unconfirmed to confirmed. This will then trigger the scoring rules for that order.
Do not process SetOrderDeliveredByColor messages at all anymore. They are no longer necessary, because the order ID is always available since it is always contained in the DS prepare message.
morxa
force-pushed
the
thofmann/auto-scoring
branch
from
9105705 to
64281e6
Compare
timn
previously requested changes
Jan 24, 2019
There was a problem hiding this comment.
Comprehensive, but I do have some concerns.
- why is the gate still in there
- why is the confirmed status not an optional field per order that is only set for clients
- it concerns me that both previous method to mark deliveries are gone. It may still be useful as a fallback (by color) and is for refbox debugging (by order). I understand it is desirable to remove all the code dealing with color-based delivery, but be careful. The by order ID functionality should be kept, debugging was also the major reason why I kept it when switching to deliver by color
I haven't actually run those changes, yet. I'd first like to discuss the implications of my remarks.
Depending on whether the delivered order was entered by color or by ID, the applied scoring rules differed. While scoring by color computed the correct score, scoring by ID computed the wrong score for the color complexity (e.g., 4 points instead of 20 points for CC2) and the pre-cap score (e.g., 0 points instead of 10 points for a C1). Fix the scoring by ID rules by applying the same rules as in the scoring by color case.
We store the time of the delivery when the DS goes into PROCESSED. This is the actual time of the delivery, not the time when we receive the delivery confirmation, which depends on the speed of the referee. Therefore, use that time to compute the score. Also rename the variable from ?game-time to ?delivery-time to make the distinction clearer.
So far we only computed the score for a late delivery if the delivery was confirmed by color. As we're getting rid of the color confirmation, also implement late delivery to ID confirmation. Also remove the code based on color confirmation.
The message will be sent by manual confirmation by the referee. This may also happen after the game has already ended. Therefore, do not constrain processing of the message on the game phase.
The message is sent to the shell so it can be confirmed by a referee.
This can be used for fields that need to be unique (e.g., an ID) but that should be an integer, not a symbol (e.g., for storing it as a uint in a protobuf message).
We need an ID so we can confirm the right delivery even if there were multiple deliveries for the same order.
The message informs the refbox shell that there has been a new, unconfirmed delivery. The referee can then confirm the correct delivery in the shell.
We no longer confirm deliveries by color but only by order. Remove the function that created and sent the message.
We no longer confirm deliveries by color but only by order. Remove the message that would confirm a delivery by color.
The new message confirms a delivery by the ID of the delivery, not by order ID. It also contains a new field 'correct'. If it is set to true, then the delivered product was as specified in the requeste order. If it is set to false, then the wrong product was delivered. The message format has changed completely, so rename the message and also change the MSG_TYPE.
The function is used by the product-delivered deftemplate and must be defined before the deftemplate. As we must load facts.clp before utils.clp, the function must be defined in facts.clp, not in utils.clp.
This replaces a good part of the menu. We now only show deliveries that have not been confirmed yet. We also no longer distinguish by order, but by delivery ID. This doesn't show the correct order configuration (i.e., the base, ring, and cap colors) yet. It's also not yet possible to report an incorrect delivery, a delivey is always assumed to be correct if confirmed.
The function find_if may return an end iterator. In that case, instead of failing, show the string "UNKNOWN" in place of the order configuration.
The function find_if may return an end iterator. In that case, instead of failing, show the string "UNKNOWN" in place of the order configuration.
|
I added commit 90ec209 that fixes the game report at the end in case there are still unconfirmed deliveries. |
As a fallback solution, we may still want to be able to set an order as delivered, even if no delivery was reported. In order to do that, add back the SetOrderDelivered message.
If we receive a SetOrderDelivered message, assert a new product-delivered fact and directly set it to confirmed.
We may want to select an order that has not been reported as delivered, which will be shown if the user selects SHOW ALL. Currently, selecting SHOW ALL will not do anything yet.
This is basically the same as the old OrderDeliverMenu. It allows to select a delivery for every order, including orders where no delivery has been reported.
The menu shows all available orders and reports a confirmed delivery for the selected order.
|
I added back the menu that would allow to select a delivery for any order by order ID. |
morxa
dismissed
timn’s stale review
Please have another look at the new changes
timn
previously requested changes
Feb 5, 2019
Instead of sending a separate message, the info about unconfirmed deliveries will be sent as part of the OrderInfo.
Instead of sending each unconfirmed delivery separately, send the list of all currently unconfirmed deliveries as part of the OrderInfo.
Instead of keeping one repeated field for each team, only use one field for both and denote the team in the sub-message.
Adapt the shell to read the unconfirmed deliveries from the OrderInfo instead of receiving and maintaining them separately.
The message format has been changed to only keep a single list of unconfirmed deliveries, adapt accordingly.
|
@timn can you please have another look? I've made the unconfirmed deliveries part of the OrderInfo. |
Yes, I'll do. Will try to get to it this week, however, may have to postpone to next week. |
timn
reviewed
Apr 9, 2019
timn
previously approved these changes
Apr 9, 2019
Co-authored-by: Tim Niemueller <niemueller@kbsg.rwth-aachen.de>
|
Thanks for the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Following robocup-logistics/rcll-rulebook#1, add a required order ID to the prepare message of the delivery station.
As we now know which order was delivered, we no longer need to have a referee that enters the delivery manually. Instead, track the delivery in the refbox and only ask the referee to confirm the delivery.
The new flow is as follows:
product-deliveredfact is asserted in the refbox, whereconfirmedis set toFALSE.UnconfirmedDeliverymessage to the shell. The message contains the team color, the delivery ID, the order ID, and the time of the delivery.I completely removed the possibility to select an order by color, as this is no longer necessary. I also removed the respective messages.
Since we mostly selected an order by color until now, the computed score was sometimes incorrect if the delivery was reported by ID. I tried to fix that but didn't test all cases.
To test:
./rcll-prepare-machine Carologistics C-DS 3 2for order 3 at gate 2.AVAILABLEto indicate that the product was put into the machine:./rcll-set-machine-state C-DS AVAILABLEThis resolves #4.