Initial Implementation for display_segment update_method replace#1820
Initial Implementation for display_segment update_method replace#1820worldpeace-germany wants to merge 17 commits intomissionpinball:devfrom
Conversation
|
In regards to the last commit fixed and clean up of color handling in segment displays In general there are some issues when defining segment display shows due to the fact how colors are being handled and how that is being known to user and if that is perceived as a logical definition flow. In segment display the default color (white) is used to expand the given color array to the right until the length of the segment display is being filled up. So for a 4 digit display, specified as default color "blue, yellow" this would end up as "blue, yellow, white, white". During initlize an empty string is sent to the segment display , each string is colored Since the length is 0, the left_pad_color is used to set the color. So it is not the default color being used, the color set now for this example is "blue, blue, blue, blue". Which on the first view doesn't matter since the display is blank, but at later points in time the code prefers to use the current_state color and not the default_color if no color is being sent, for example if the other digits cannot display blue they will simply stay blank. In other parts of the code, if you start a transition the given colors are being expanded , where expanded could as well mean shortend to the length of the text and not the length of the display, so later the empty places are being padded with acolor again (see above). If the colors need to be expanded then this is done by taking the most right color and reuse it for all the other text to the right of this position (so no default color being used). If a transition text is being used and no color is specified, then the color of the first character of the new text is being used (again not the default color). In general we have default color, sometimes padded to the left and sometimes padded to the right. Question is if the user can follow that behavior or if it is over complicated and thus introduces user mistakes. The PR will change some of the behavior to make it more consistent
visible and not that because of a wrong color invisible text is shown and confuses the user. But the PR not only makes the handling more consistent but I would consider it a bug if a color is used for padding which might be able to be used for the digits of that segment display. Once the PR is accepted I will update the documentation to explain the color handling in detail for the users. |
avanwinkle
left a comment
avanwinkle
left a comment
There was a problem hiding this comment.
This is a lot of great work, thanks for tackling this part of the code that most people don't ever look at.
I've noted in my comments that I think we could strip down a lot of the color-filling logic because I don't think it should be MPF's job to try and figure out what the designer means. The designer should be clear and provide the correct number of colors.
As far as priority, I think it's worth keeping for replace mode because there may be situations where multiple modes have segment_display_players and the highest priority should have precedence. Otherwise designers will have a lot of manual work to do if they want to suppress lower-priority mode text from showing up during a higher-priority mode (e.g. multi ball)
| ############################### | ||
|
|
||
| # Handle new and previous text | ||
| if self._previous_text: |
There was a problem hiding this comment.
For pythonic convenience, this could be rewritten as:
previous_text = self._previous_text or ""
Same below with previous_color
| self._previous_color = color # Save the new color as the next previous color | ||
|
|
||
| if transition or self._previous_transition_out: | ||
| if transition: #if transition exists, then ignore transition_out of previous text/color |
There was a problem hiding this comment.
This second nested block could be removed by passing both possible values as arguments.
if transition or self._previous_transition_out:
transition_conf = TransitionManager.get_transition(self.size,
self.config['integrated_dots'],
self.config['integrated_commas'],
self.config['use_dots_for_commas'],
transition or self._previous_transition_out)
self._previous_transition_out = transition_out or None
| self.config['default_transition_update_hz'], flashing, flash_mask) | ||
|
|
||
| else: #No transition configured | ||
| if transition_out: #in case transition_out is set we need to preserve it for the next step |
There was a problem hiding this comment.
The gotcha here is that if there is no transition_out on this transition but the previous transition did have an out, then self._previous_transition_out will still have the value of the last transition out. Best to be explicit and not use an if statement.
self._previous_transition_out = transition_out or None
Which is also in the above if block, so it could be called just once at the end of this method outside of the if/else block.
| """Remove entry from text stack.""" | ||
| if self.config['update_method'] != "stack": | ||
| self.info_log("Segment display 'remove' action is TBD.") | ||
| self.add_text_entry("", self._previous_color, FlashingType.NO_FLASH, "", None, None, 100, key) |
There was a problem hiding this comment.
This is a good use of an existing method to clear out a display, but the key part is misleading because the argument is passed to the method to define which key to remove, but it's actually adding empty text with that key.
I would suggest instead that this behavior check the current text to see if it has the requested key, and if so remove the text and pass an empty key, and if not do nothing.
There was a problem hiding this comment.
I would tend to disagree on this point. But maybe I misunderstood something
here. I tried to change the code as little as possible that is why we have
this early return (which I would usually try to avoid), maybe that is
misleading. Initially that function was only implemented for the stack case
(and that implementation has not changed).
For the replace case I would claim neither key nor priority is relevant since
we don't have a stack where we can remove something based on a key (or sort
based on a priority). Of course a key is passed as well for the replace case,
but there is nothing you can really do with it (in the replace case). So
removal in that case means rather emptying the display. Especially since that
remove method is as well called when the context is being removed (when a show
gets the stop command).
On a side note: I am working on one fix for the show stop since if you stop a
show then the current transition is still being played (in the replace case)
and a transition_out would be as well played. But it still empties the display.
If you really believe that the key is relevant for the replace case then please rephrase your feedback. Happy to understand it better and to improve.
There was a problem hiding this comment.
Here's a use case I'm picturing where key and priority would be helpful, let me know what you think.
A player starts a non-exclusive hurry-up mode (priority 1000) with a jackpot on the left loop, and that hurry-up mode sends the text "HIT LEFT LOOP" to a display. Instead of going for the hurry-up, the player instead starts a multi ball (priority 5000) and the multi ball mode sends the text "JACKPOT: 5M" to the display. During the multi ball, the hurry-up mode times out and ends.
By default, the hurry-up mode is set to remove the "HIT LEFT LOOP" text from the display, so it will send a command to the displays to replace the text with empty string. Without a key, the display doesn't know that the text being shown is not the text that's expected to be removed, and will instead remove the "JACKPOT: 5M" text from the display even though the multi ball is still running. As a result, the multi ball will have a blank display.
Reverse the situation and start the multi ball before the hurry-up. The multi ball mode is higher priority so its display text should have precedence, and the hurry-up display text should be ignored. Otherwise the multi ball "JACKPOT: 5M" will turn into "HIT LEFT LOOP" and then disappear after the hurry-up, again leaving the multi ball with a blank display.
I realize that this type of use case is exactly what the stack mode is meant to do, but I believe there is value in replace also respecting keys and priorities because that creates more consistent behavior in the game. Designers will have the option to override the behavior by specifying keys and priorities if they want to, but regardless the behavior will be the same every time. Ignoring keys and priorities will yield different displays based on timing, which is not ideal.
Thoughts?
There was a problem hiding this comment.
The described use case is very valid and is very much a use case for the stack case. The initial implementation of replace
indicates that the text is written straight to the display. Can you describe in words what would be the logic of replace if it includes key and priority and where this behavior differs from the stack case? I simply don't see the logic of such an implementation.
There was a problem hiding this comment.
You're right, I'm overthinking it and trying to keep stack-like functionality where it doesn't belong. The concept of replace doesn't need to have any considerations for priority or key or what's currently on the display, and should always overwrite when called.
avanwinkle
left a comment
avanwinkle
left a comment
There was a problem hiding this comment.
Oops, I meant to post that review as Request Changes, not Comment. See above for requested changes :)
|
|
@avanwinkle May I check if you believe that something is missing to accept that PR? Feedback to improve is of course more than welcome. |
avanwinkle
left a comment
avanwinkle
left a comment
There was a problem hiding this comment.
My apologies for letting this languish for so long, thanks for your patience. A few small notes and one previous unaddressed comment, please take a look. Thanks!
|
|
||
| for display, keys_dict in instance_dict.items(): # keys_dict key is the show key, the value is a boolean (with yet unknown usage) | ||
| if(keys_dict): #depending on the situation the keys_dict might be empty, still need to clear the display | ||
| for key in dict(keys_dict).keys(): |
There was a problem hiding this comment.
In what situation would keys_dict not be a dict? It feels weird to convert it to dict here instead of for key in keys_dict.keys()
| if(keys_dict): #depending on the situation the keys_dict might be empty, still need to clear the display | ||
| for key in dict(keys_dict).keys(): | ||
| display.clear_segment_display(key) | ||
| if instance_dict[display][key] is not True: |
There was a problem hiding this comment.
I'm not really sure what this means, in what scenario would the value be True and what would it be otherwise? Could you add a comment?
There was a problem hiding this comment.
I think the weirdness of this here comes from copying _remove. I think the resolution here would be to make a second _remove function (with an additional word or two of context in the name) that looks like the existing _remove function, so the quirks of the dict management and delay removal will be more apparently parallel (and possibly refactorable)
|
Looks good! Some linter/formatting fixes and some tests needs updated, but otherwise a strong improvement! |
|
|
@avanwinkle I hope I have implemented all feedback. Can you take a look to check if I have missed something? I am aware that, test cases are failing, that is mainly due to the fact that we changed the behavior on what colors the system tried to "guess" in the past. We have agreed to simplify it (I have explained that in my PR for the documentation). I have to admit I have no idea how to fix the testing code. If someone gives me a hand I am willing to learn and fix it. |
|
1 similar comment
|
|
@avanwinkle Finally found the time to get into Python testing and fixed all the failing tests. Sorry that it took a while, but time is limited and there was a learning curve... If you have time please take a look and let me know if you spot something before the PR can be accepted. |
|
comments improved
comments improved
not needed parenthesis removed
|
|
@bosh may I check with you since you "touched" my PR what I can do to get it accepted? From my pov it is all complete. Even if there is a reason why it will never be accepted it would be nice to get some feedback. Thanks! |
|
No worries! I've recently gained commit abilities, so I've been trying to clear out old things by merging or killing :) It looks like it's all green, but I haven't really dug in on the content here yet. I see in the first reviewed file, |
|
@bosh Thanks for looking into it. I believe I have fixed all comments from Anthony. If I have missed something then of course I will look into it at the moment I simply don't see what I might have missed. Anyways, if you take a look and find something then please let me know and I will update the code where needed. |
|
Yeah it looks like just two minor nits in the file I mentioned. It looks like the very long comment chain is actually resolved by doing nothing. I'll give it a once over this weekend, since I don't want to merge anything I don't understand. |
3 participants
config_spec.yaml:
changed default from
not_settooff, leftnot_setin enum though I believe it can be removed. I couldn't see why not_set is a good default. If really needed for the changes in update_methodreplaceI can add code to exchangenot_setforoff.segment_display:
Only changed code for update_method
replace,stackshould behave like before. I tested with various segment displays shows and it seems to behave well. Note:replacethepriorityparameter is not used (and doesn't make sense if you don't have a stack)replace, but completely misbehave if usingstackreplacein herempf/mpf/devices/segment_display/segment_display.py
Line 139 in 35a84ba
colorchanges since my display is single color (though would believe it should work if it works forstack)flashing,transition,transition_out,expireparameter tested (and working)If the PR gets accepted I would do some additons to mpf-docs 0.80. Just would need to know how to contribute version specific changes.