Core, MultiServer, CommonClient: Store Hint Priority & the found attribute together in an IntFlag for efficiency #4721
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -11,12 +11,87 @@ | |||||
| from Utils import ByValue, Version | ||||||
|
|
||||||
|
|
||||||
| class HintStatus(ByValue, enum.IntFlag): | ||||||
| # Lower 5 bits: Priorities as integers | ||||||
| HINT_PRIORITY_UNSPECIFIED = 0 # For readable code | ||||||
| HINT_PRIORITY_NO_PRIORITY = 1 | ||||||
| HINT_PRIORITY_AVOID = 2 | ||||||
| HINT_PRIORITY_PRIORITY = 3 | ||||||
| PRIORITY_MASK = 0b11111 | ||||||
|
|
||||||
| # Bits 6+: Technical status | ||||||
| HINT_FOUND = 0b10000000 | ||||||
|
Comment on lines
+15
to
+23
There was a problem hiding this comment. So if I understand correctly, bits 0-4 are used for priority (encoded as integer, not as bit flag), bits 5-6 are unused and bit 7 is for found status encoded as a bit flag? I get that we want to be efficient with the multiserver memory, but that looks like a very complex encoding to save 1 boolean per hint... This has probably been discussed already, I missed the discord conversations lately. Feel free to dismiss. There was a problem hiding this comment. I have plans for bits 5 and 6 fwiw, but yes, it's all about memory. |
||||||
|
|
||||||
| # For "compatibility", only used in CommonClient, thus should not affect memory for MultiServer | ||||||
| HINT_UNFOUND_LEGACY = OLD_HINT_FORMAT = 0b1000000000000000 | ||||||
|
There was a problem hiding this comment. What would you think of using bit shifts here? I think it is easier to understand that the bit 15 is set with something like
Suggested change
(same comment for |
||||||
|
|
||||||
| @property | ||||||
| def priority(self): | ||||||
|
There was a problem hiding this comment. You might want to add the return types here, same for the other methods.
Suggested change
There was a problem hiding this comment. Can you do that? I thought this causes a problem and you need typing.Self for it or sth There was a problem hiding this comment. Yeah, I guess you're right. Looking at See https://docs.python.org/3/library/typing.html#typing.Self There was a problem hiding this comment. Well, that feature was introduced in Python 3.11, and we still support 3.10 :D There was a problem hiding this comment. We have There was a problem hiding this comment. That's true. I wonder if there are other instances of this in this file, if there are I feel like that should maybe be its own PR, if not then I can add it for sure There was a problem hiding this comment. Typing in this file seems not exactly consistent. Some methods are typed correctly, some are missing return types, some are missing args... I would not hold on adding typing to the methods of Hint/HintStatus because of that since you already changed those lines, but that's your call. |
||||||
| return self & HintStatus.PRIORITY_MASK | ||||||
|
|
||||||
| def with_priority(self, priority: HintStatus): | ||||||
| return (self & ~HintStatus.PRIORITY_MASK) | (priority & HintStatus.PRIORITY_MASK) | ||||||
|
|
||||||
| @property | ||||||
| def found(self): | ||||||
| return HintStatus.HINT_FOUND in self | ||||||
|
|
||||||
| def with_found(self, found: bool): | ||||||
| return (self & ~HintStatus.HINT_FOUND) | (found * HintStatus.HINT_FOUND) | ||||||
|
|
||||||
| @classmethod | ||||||
| def from_found_and_priority(cls, found: bool, priority: HintStatus): | ||||||
| return cls((HintStatus.HINT_FOUND * found) | priority) | ||||||
|
|
||||||
| def as_display_status(self): | ||||||
| if self.found: | ||||||
| return HintStatus.HINT_FOUND | ||||||
| if HintStatus.OLD_HINT_FORMAT in self: | ||||||
| return HintStatus.HINT_UNFOUND_LEGACY | ||||||
|
|
||||||
| # Make sure we're only returning statuses that are actually displayable | ||||||
| return next( | ||||||
| (priority for priority in status_names if priority == self.priority), | ||||||
| self.HINT_PRIORITY_UNSPECIFIED | ||||||
| ) | ||||||
|
|
||||||
| @property | ||||||
| def changeable(self): | ||||||
| return not self.found and not HintStatus.OLD_HINT_FORMAT in self | ||||||
|
|
||||||
|
|
||||||
| status_names_brackets: typing.Dict[HintStatus, str] = { | ||||||
| HintStatus.HINT_FOUND: "(found)", | ||||||
| HintStatus.HINT_PRIORITY_PRIORITY: "(priority)", | ||||||
| HintStatus.HINT_PRIORITY_AVOID: "(avoid)", | ||||||
| HintStatus.HINT_PRIORITY_NO_PRIORITY: "(no priority)", | ||||||
| HintStatus.HINT_PRIORITY_UNSPECIFIED: "(unspecified)", | ||||||
| HintStatus.HINT_UNFOUND_LEGACY: "(not found)" | ||||||
| } | ||||||
| status_names: typing.Dict[HintStatus, str] = { | ||||||
| HintStatus.HINT_FOUND: "Found", | ||||||
| HintStatus.HINT_PRIORITY_UNSPECIFIED: "Unspecified", | ||||||
| HintStatus.HINT_PRIORITY_NO_PRIORITY: "No Priority", | ||||||
| HintStatus.HINT_PRIORITY_AVOID: "Avoid", | ||||||
| HintStatus.HINT_PRIORITY_PRIORITY: "Priority", | ||||||
| HintStatus.HINT_UNFOUND_LEGACY: "Not Found" | ||||||
| } | ||||||
| status_colors: typing.Dict[HintStatus, str] = { | ||||||
| HintStatus.HINT_FOUND: "green", | ||||||
| HintStatus.HINT_PRIORITY_PRIORITY: "plum", | ||||||
| HintStatus.HINT_PRIORITY_AVOID: "salmon", | ||||||
| HintStatus.HINT_PRIORITY_NO_PRIORITY: "slateblue", | ||||||
| HintStatus.HINT_PRIORITY_UNSPECIFIED: "white", | ||||||
| HintStatus.HINT_UNFOUND_LEGACY: "red", | ||||||
| } | ||||||
| status_sort_weights: dict[HintStatus, int] = { | ||||||
| HintStatus.HINT_FOUND: 0, | ||||||
| HintStatus.HINT_PRIORITY_UNSPECIFIED: 1, | ||||||
| HintStatus.HINT_PRIORITY_NO_PRIORITY: 2, | ||||||
| HintStatus.HINT_PRIORITY_AVOID: 3, | ||||||
| HintStatus.HINT_PRIORITY_PRIORITY: 4, | ||||||
| HintStatus.HINT_UNFOUND_LEGACY: 5, | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
| class JSONMessagePart(typing.TypedDict, total=False): | ||||||
|
|
@@ -278,7 +353,7 @@ def _handle_entrance_name(self, node: JSONMessagePart): | |||||
| return self._handle_color(node) | ||||||
|
|
||||||
| def _handle_hint_status(self, node: JSONMessagePart): | ||||||
| node["color"] = status_colors[HintStatus(node["hint_status"]).as_display_status()] | ||||||
| return self._handle_color(node) | ||||||
|
|
||||||
|
|
||||||
|
|
@@ -313,51 +388,37 @@ def add_json_location(parts: list, location_id: int, player: int = 0, **kwargs) | |||||
| parts.append({"text": str(location_id), "player": player, "type": JSONTypes.location_id, **kwargs}) | ||||||
|
|
||||||
|
|
||||||
| def add_json_hint_status(parts: list, hint_status: HintStatus, text: typing.Optional[str] = None, **kwargs): | ||||||
| if text is None: | ||||||
| text = status_names_brackets[hint_status.as_display_status()] | ||||||
| parts.append({"text": text, "hint_status": hint_status, "type": JSONTypes.hint_status, **kwargs}) | ||||||
|
|
||||||
|
|
||||||
| class Hint(typing.NamedTuple): | ||||||
| receiving_player: int | ||||||
| finding_player: int | ||||||
| location: int | ||||||
| item: int | ||||||
| entrance: str = "" | ||||||
| item_flags: int = 0 | ||||||
| status: HintStatus = HintStatus.from_found_and_priority(False, HintStatus.HINT_PRIORITY_UNSPECIFIED) | ||||||
|
|
||||||
| @property | ||||||
| def found(self): | ||||||
| return self.status.found | ||||||
|
|
||||||
| def re_check(self, ctx, team) -> Hint: | ||||||
| if self.found: | ||||||
| return self | ||||||
| found = self.location in ctx.location_checks[team, self.finding_player] | ||||||
| if found: | ||||||
| return self._replace(status=self.status.with_found(True)) | ||||||
| return self | ||||||
|
|
||||||
| def re_prioritize(self, status: HintStatus) -> Hint: | ||||||
| if status.priority == self.status.priority: | ||||||
| return self | ||||||
| return self._replace(status=status.with_priority(status.priority)) | ||||||
|
|
||||||
| def __hash__(self): | ||||||
| return hash((self.receiving_player, self.finding_player, self.location, self.item, self.entrance)) | ||||||
|
|
@@ -390,6 +451,22 @@ def local(self): | |||||
| return self.receiving_player == self.finding_player | ||||||
|
|
||||||
|
|
||||||
| # From what I can tell, all the methods to make unpickling backwards compatible don't work for NamedTuples. | ||||||
| # This is because you can't override __getattributes__ or __new__ on a NamedTuple. | ||||||
| # The only approach I found was to reroute the Unpickler to a dummy class with a custom __new__ in its find_class. | ||||||
| class CompatibleHint: | ||||||
| def __new__(cls, *args, **kwargs): | ||||||
| if type(args[4]) == bool: | ||||||
| # old hint format | ||||||
| legacy_found = args[4] | ||||||
|
|
||||||
| # No compatibility for old priority system, just revert to unspecified | ||||||
| new_hint_status = HintStatus.from_found_and_priority(legacy_found, HintStatus.HINT_PRIORITY_UNSPECIFIED) | ||||||
|
|
||||||
| args = (*args[:4], *args[5:-1], new_hint_status) | ||||||
| return Hint.__new__(Hint, *args, **kwargs) | ||||||
|
|
||||||
|
|
||||||
| class _LocationStore(dict, typing.MutableMapping[int, typing.Dict[int, typing.Tuple[int, int, int]]]): | ||||||
| def __init__(self, values: typing.MutableMapping[int, typing.Dict[int, typing.Tuple[int, int, int]]]): | ||||||
| super().__init__(values) | ||||||
|
|
||||||
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.
The fact that useful trap is in many cases used to mark a trap as "never exclude", would still make these wanted to be avoided. Then again, if someone uses the flag to mean "actually useful", then this would not be desired. Neither way here is correct, but, that's entirely the fault of the
never_exclude/usefulbullshit, so, 🤷♀️ I guess this is fineAlso if we go with the change of "default to unspecified, add commands for other priorities", then this is obsolete anyway- which I think is a change strongly worth making.