Skip to content

Made Events work with @requirement and RB bugfixes - #201

Open
nicopop wants to merge 16 commits into
mainfrom
rules.py-misc-changes
Open

Made Events work with @requirement and RB bugfixes#201
nicopop wants to merge 16 commits into
mainfrom
rules.py-misc-changes

Conversation

@nicopop

@nicopop nicopop commented Feb 2, 2026

Copy link
Copy Markdown
Contributor

I got inspired by what I said on discord in manual-core-dev channel and I went and really looked at the rules.py file
things included in this PR so far:

  • Replaced the old state.counts call loops with calls to either state.has or state.has_from_list
    • fixes events not being accessible via @category, :half/:all
  • Added Some type anotations to make mypy happy
  • Made item_count convert back to item_name if the count is an unknown string

that last point can be reverted/modified if we still want it to raise an exception instead

@nicopop nicopop self-assigned this Feb 2, 2026
silasary
silasary previously approved these changes Feb 5, 2026
@nicopop

nicopop commented Feb 14, 2026

Copy link
Copy Markdown
Contributor Author

Actuallly im putting this PR on pause until the rule builder one is merged (link here once its created)

@nicopop nicopop added the blocked by feedback This PR is blocked by feedback in the PR label Feb 14, 2026
@silasary

Copy link
Copy Markdown
Contributor

#203 is the rule builder PR

@nicopop nicopop mentioned this pull request Feb 14, 2026
@nicopop

nicopop commented Mar 30, 2026

Copy link
Copy Markdown
Contributor Author

with pr #203 merged ill adapt this PR to the new code,
I don't have time this week but I should work on this soonish™

@nicopop

nicopop commented Mar 31, 2026

Copy link
Copy Markdown
Contributor Author

with pr #203 merged ill adapt this PR to the new code, I don't have time this week but I should work on this soonish™

I didnt mean to lie when I said that, I just got inspired when doing the preparatory merge from main to fix the conflicts

@nicopop nicopop removed the blocked by feedback This PR is blocked by feedback in the PR label Mar 31, 2026
@nicopop nicopop changed the title Rules.py misc changes Made Events work with @requirement and RB bugfixes Apr 9, 2026
Comment thread src/Rules.py
for exitRegion in multiworld.get_region(region, player).entrances:
extra = extra_entrance_rules.get(exitRegion.name, {})
rb_rule = construct_rule_from_string(regionMap[region])
area = regionMap[region]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we're reassigning ["name"], this should probably be a .copy()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its what we did before RB, I just moved it here so errors after this part of the code have a name and are detected as a region.
one thing we could do instead is move that code to be in regions.py so that they use the exact region name instead of region A to B thing they have currently (if i remember correctly)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm aware that it's something we're already doing, that's why I approved regardless.

But when you moved it it brought it to my attention as a thing we probably shouldn't be doing.

Comment thread src/Rules.py
@nicopop

nicopop commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Found out recently while doing a Manual with all of my PRs that with RB there are no call to construct_logic_error for unbalanced parentheses (which causes an exception in the code) idk If I should include the fix for it here or in another PR after this one

@nicopop

nicopop commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Found out recently while doing a Manual with all of my PRs that with RB there are no call to construct_logic_error for unbalanced parentheses (which causes an exception in the code) idk If I should include the fix for it here or in another PR after this one

The reason I wonder if I should do it in another PR is that while fixing this I found other issues with the way Manual react when there's an invalid item/function with a number in it
the fix for that bug could be in a following PR or it could be in this one


A require of |Chun-Li| or {YamlCompare(Example_Choice == 1) doesnt trigger any exception
it goes in the following order

  1. RB fails because it cannot be tokenized
  2. manual fallback to checkRequireStringForArea
  3. it also fail at detecting the "function" since its invalid
  4. infix_to_postfix does it thing skip all the non-numeric character
  5. it resolve to 0|1 because of the == 1
  6. manual see that as valid and voila a missed typo

the fix for this I have locally is to make infix_to_postfix raise a valueError on non-numeric char
and then construct a more "correct" logic error
it does mean the exception checking of evaluate_postfix would probably become useless since they would now be done in infix_to_postfix and that we probably could modify the LogicErrorSource values name

def infix_to_postfix(expr: str, location: dict) -> str:
            ...
            elif c == ")":
                while stack and stack[-1] != "(":
                    postfix += stack.pop()
                stack.pop()
            else:
                # added this here since '|Chun-Li| or {YamlCompare(Example_Choice == 1)' is valid because of the 1 at the end
                raise ValueError(f"Invalid Character '{c}' in expression '{expr}', it should be either a number or a parentheses")

        while stack:
            postfix += stack.pop()
    except ValueError as ex:
        text = str(ex)
        if "'{'" in text or ")}" in text:
            raise construct_logic_error(location, LogicErrorSource.EVALUATE_STACK_SIZE)

        raise construct_logic_error(location, LogicErrorSource.EVALUATE_POSTFIX)
    except Exception:
        ...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants