Conversation
| "name": "Beat the Game - Deadpool", | ||
| "category": ["Unlocked Teams", "Right Side"], | ||
| "requires": "|Deadpool| or {YamlEnabled(free_deadpool)}" | ||
| "requires": "|Deadpool| or {YamlEnabled(start_with_deadpool)}" |
There was a problem hiding this comment.
Yes, this was a logic error that I discovered when implementing YamlEnabledRule
| # if isinstance(value, bool): | ||
| # return value | ||
| # name = self.access_rule.__name__ | ||
| # raise ValueError(f'Unexpected return value {value} from {name}') |
There was a problem hiding this comment.
Let me know if we want this here, or if I should remove it. I consider it a failed experiment, but it might be needed when we drop support for pre-0.6.7 and make RB rules mandatory?
There was a problem hiding this comment.
personaly I would prefer we try and keep back compat somewhat but you know a lot more than me on how rules works.
in my ideal world there would be a way for us to detect incompatible rules and somehow make them work with the rule builder even if its only to explain that the logic require "functionPotato" to be true
There was a problem hiding this comment.
On one hand, yes, absolutely.
On the other hand, permanently maintaining two separate codepaths is maintenance hell and technical debt.
Making this thing work is a much better way of doing backwards compatibility long term, as opposed to all the if use_rulebuilder branches and the legacy stringsub code.
| break | ||
|
|
||
| if func and inspect.signature(func).return_annotation is str: | ||
| # I'm assuming that functions that return strings don't need states. |
There was a problem hiding this comment.
why not let them have state?
There was a problem hiding this comment.
State does not exist this early
There was a problem hiding this comment.
Under the old system, these functions are evaluated during generation, as it's trying to resolve item placement.
But this code is happening during set_rules, while the locations are still being built.
If we want a universal handler that can handle rules with (state) -> bool, we'd need to get the commented fallback rule at the bottom working.
| rule_class = func | ||
| break | ||
|
|
||
| if func and inspect.signature(func).return_annotation is str: |
There was a problem hiding this comment.
I had this idea while reading the code:
could we support functions that return valid rules? I could see someone doing some logic to build a rule and have that sent to us?
I have no idea how possible that would be its just an idea
There was a problem hiding this comment.
by that I mean return something like Has(X) & Has(Y)
There was a problem hiding this comment.
Technically yes? But it's easier to just make a custom rule the right way:
https://github.com/silasary/APxiv/blob/287af9a70ac1d73c128949b6be066e36656876fc/src/hooks/Rules.py#L53-L59
There was a problem hiding this comment.
I very much want existing functions to continue to work without Builder, and the moment they can return a rule, that breaks.
While Rule Builder should be much faster for manual logic because manual logic currently re-parses each rule from json on every single call and evaluates every single part of a rule without any boolean short-circuiting, the performance improvement for the ffxiv manual is likely to be much more than other manuals because the ffxiv manual uses a lot of hook functions, and manual's current function parsing is extra slow to perform on every single access rule call. I rewrote manual logic to compile all rules to lambdas a while back (https://github.com/Mysteryem/ManualForArchipelago/pull/1/changes#diff-a25c303033861df67335b2a78c209196808d8d93e059588997e9f5422bad35e8), but decided not to progress further until Rule Builder, since I expected Rule Builder would be the way forward for Manual. |
|
Oh, I am well aware that the FFXIV manual is an outlier when it comes to complexity and room for improvement. That's why I tested with it :) |
|
With this being "an absolute monstrosity of a refactor", this feels too high risk to merge without a more recent stable release first (even though it includes tangible performance benefits for generation time). This could have a long time in unstable working out the issues, and I don't believe it should block the previous unstable improvements from being released in the meantime. |
|
100% agreed. I think we get a new stable out soon, and then release an unstable with this shortly afterwards? |
|
New stable is out. Shall we? |
4 participants
This is an absolute monstrosity of a refactor, but it is absolutely worth it.
I've stress-tested it on the ffxiv manual, over the past few months, and it can shave upwards of a minute off generation time.
That said, it is effectively a full rewrite of a lot of our core logic, so please test it properly.
Needs to be run against latest AP source, or it won't do anything at all.