Initial Implementation for display_segment update_method replace #1820
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. |
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.
| @@ -171,7 +226,7 @@ def add_text(self, text: str, priority: int = 0, key: str = None) -> None: | |||
| def remove_text_by_key(self, key: Optional[str]): | |||
| """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.
| current_length = len(char_list) | ||
| # Adujust the color array if needed | ||
| color_length = len(colors) | ||
| if color_length > display_size: |
There was a problem hiding this comment.
How much do we really want to handle here? I think it is reasonable for a designer to send one color (for all segments) or a list of colors (one for each segment), and those are the two options.
It doesn't seem reasonable to expect MPF to "correctly" handle coloring a seven segment display when you send it four colors or something 🤷. I think we should strip down all this color-filling logic and throw an exception if the list of colors is neither 1 nor the display size. Unless I'm missing a use case?
There was a problem hiding this comment.
Very true, that is what I asked myself as well. I tried to be minimal invasive with my code changes, so that is what I came up with. But it seems we have the same understanding here and thus I am happy to change the color handling. Will provide a new PR in the next days including all the other feedback you gave.
| char_list.insert(0, DisplayCharacter(SPACE_CODE, False, False, left_pad_color)) | ||
| uncolored_chars.insert(0, (SPACE_CODE, False, False)) | ||
|
|
||
| for _ in range(len(uncolored_chars)): |
There was a problem hiding this comment.
By convention, using the underscore as a variable in a loop is meant to indicate that the variable is never used, i.e. the loop code doesn't care which iteration it's on. If you are going to use that variable, a letter is preferred (either the generic i or the first letter of what's being iterated, in this case c).
There was a problem hiding this comment.
On second thought, that's not necessary because uncolored_chars is iterable, so you can use enumerate() to get both the index and the value (which saves time on all the lookups in DisplayCharacter).
for i, char in enumerate(uncolored_chars):
color = colors[i]
char_list.append(DisplayCharacter(char[0], char[1], char[2], color))
| @@ -145,7 +148,14 @@ def add_text_entry(self, text, color, flashing, flash_mask, transition, transiti | |||
|
|
|||
| This will replace texts with the same key. | |||
| """ | |||
|
|
|||
| if len(color) == 0: | |||
There was a problem hiding this comment.
This could be more readable (and do the same thing) with a generic truthiness check:
if not color:
color = self._current_state.text.get_colors()
|
|
|
@avanwinkle May I check if you believe that something is missing to accept that PR? Feedback to improve is of course more than welcome. |
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.