Core: Added Handler for math operations and check for math operator in Triggers. #4022

Vertraic · 2024-10-02T08:51:15Z

What is this fixing or adding?

Adds the ability to perform basic arithmetic operations in trigger options, used by adding a * to the end of the option you are performing arithmetic to determine. Values are restricted to numeric values between -1,000,000 and 1,000,000 for security.

For example:
first_bowser_star_door_cost*: "amount_of_stars / 10" in Super Mario 64 would set the first bowser door star requirement to 1/10 the total number of stars. Weighted lists of options are also accepted:

basement_star_door_cost*:
  "amount_of_stars / 5": 2
  "amount_of_stars - 10": 3

This would make the basement door require either 1/5 of the stars, or 10 less than the total number of stars.

How was this tested?

Tested using the following triggers/test values, and checking output with debugger. Then testing to make sure following triggers worked, weights were appropriately chosen, and the final file had the settings that would be expected.

  triggers:
    - option_category: DLCQuest
      option_name: coinsanity
      option_result: none
      options:
        DLCQuest:
          coinsanity: coin
          coinbundlequantity*: 
            "test1+test2-test4": 1
            "test1*test4": 20000
            "test2/test4": 3
            "1.37/test2": 4
            "test4/-157.3": 5
            "test1/2": 6
            "test4-test2": 7
            "-2*-3": 8
            "2": 9
            "4-2": 1
    - option_category: DLCQuest
      option_name: coinbundlequantity
      option_result: 1
      options:
        DLCQuest:
          campaign: both

  # General

  test1: 3
  test2: 7
  test3: 14
  test4: 9

If this makes graphical changes, please attach screenshots.

None.

Exempt-Medic · 2024-10-02T15:35:39Z

Generate.py

+        elif value_list[1] == '*':
+            new_dict.update({value_list[0] * value_list[2]: value})
+        else:
+            new_dict.update({value_list[0] / value_list[2]: value})


I feel like this should be using integer division (floats generally cause problems)

Fixed by explicitly choosing division/floor division based on whether a float value was involved to begin with. Float results rounded to 6 decimal places.

Exempt-Medic · 2024-10-02T15:36:50Z

Generate.py

+    for name, value in options.items():
+        value_list = []
+        if not isinstance(name, str):
+            raise Exception(f'Mathematical trigger malformed: {name} is not a string.')


Following the style guide, these should use doublequotes

Suggested change

raise Exception(f'Mathematical trigger malformed: {name} is not a string.')

raise Exception(f"Mathematical trigger malformed: {name} is not a string.")

Exempt-Medic · 2024-10-02T15:38:23Z

Generate.py

+            raise Exception(f'Cannot perform arithmetic, wrong number of arguments: {math_options}')
+        if split_name[1] not in ['+', '-', '*', '/']:
+            raise Exception(f'Cannot perform arithmetic, unknown operator in option: {split_name[1]}')
+        for x in range(0, 2, 2):


Isn't this literally just 0?

No idea how I got the right results from that... Should have been 0, 3, 2

Exempt-Medic · 2024-10-02T15:39:21Z

Generate.py

+                    weights[split_name[x]] = get_choice(split_name[x], weights)
+                    split_name[x] = str(weights[split_name[x]])
+                if not split_name[x].isnumeric():
+                    raise Exception(f'Cannot perform arithmetic, non-numeric value found for : {name.split(" ")[0]}')


Honestly, you can probably remove the colon entirely, right?

Suggested change

raise Exception(f'Cannot perform arithmetic, non-numeric value found for : {name.split(" ")[0]}')

raise Exception(f'Cannot perform arithmetic, non-numeric value found for: {name.split(" ")[0]}')

Exempt-Medic · 2024-10-02T15:40:45Z

Generate.py

+            else:
+                value_list[x] = int(split_name[x])
+            if value_list[x] > 1_000_000 or value_list[x] < -1_000_000:
+                raise Exception(f'Arithmetic Option out of bounds: {value_list[0]}, {value_list[2]} must < 7 digits.')


This does not read well

Suggested change

raise Exception(f'Arithmetic Option out of bounds: {value_list[0]}, {value_list[2]} must < 7 digits.')

raise Exception(f'Arithmetic Option out of bounds: {value_list[0]}, {value_list[2]} must be between -1000000 and 10000000')

Exempt-Medic · 2024-10-02T15:43:34Z

Generate.py

+                value_list[x] = float(split_name[x])
+            else:
+                value_list[x] = int(split_name[x])
+            if value_list[x] > 1_000_000 or value_list[x] < -1_000_000:


This might be clearer 🤷

Suggested change

if value_list[x] > 1_000_000 or value_list[x] < -1_000_000:

if not (-1_000_000 < value_list[x] < 1_000_000):

and fixed range error and division type issues. Mostly as advised by Exempt-Medic

…o.git into MathTriggers

beauxq · 2024-10-03T14:57:31Z

Generate.py

+                        temp_name = option_type.rsplit("*", 1)
+                        if len(temp_name) == 2 and temp_name[1] == "":
+                            temp_name = temp_name[0]


Is this a way of writing if option_type.endswith("*"): ?

Yeah, I was working on something else using splits when I wrote this and I guess my brain locked in on that method.

beauxq · 2024-10-03T15:06:31Z

Generate.py

@@ -378,6 +378,50 @@ def roll_linked_options(weights: dict) -> dict:
    return weights


+def handle_trigger_math(math_options: Union[dict, string], option_name: str, weights: dict):


Suggested change

def handle_trigger_math(math_options: Union[dict, string], option_name: str, weights: dict):

def handle_trigger_math(math_options: Union[dict, str], option_name: str, weights: dict):

beauxq · 2024-10-03T15:22:07Z

Generate.py

+        value_list = []
+        if not isinstance(name, str):
+            raise Exception(f"Mathematical trigger malformed: {name} is not a string.")
+        split_name = name.split(" ")


Is this requiring space around the operator? Is it not valid to use "amount_of_stars-10" ?
That seems kind of strict to me.
If this is the case, it should probably be noted in the documentation that spaces are required.
But I think it would be better if spaces weren't required. I suspect people will run into this issue.

Rewrote using regex. Now handles spaces, no spaces, and negative constants.

beauxq · 2024-10-03T18:30:25Z

Generate.py

+            if not split_name[x].isnumeric():
+                if split_name[x] in weights:
+                    weights[split_name[x]] = get_choice(split_name[x], weights)
+                    split_name[x] = str(weights[split_name[x]])
+                if not split_name[x].isnumeric():
+                    raise Exception(f"Cannot perform arithmetic, non-numeric value found for {name.split(' ')[0]}")
+            if len(split_name[x].split(".")) > 1:
+                value_list[x] = float(split_name[x])


It looks to me like this attempt at float support won't work.

>>> "3.5".isnumeric() False

also negative numbers:

>>> "-3".isnumeric() False

Fixed using regex to check that it is a numeric value.

Vertraic · 2024-10-04T07:42:49Z

Generate.py

+        options = {str(options): 1}
+    new_dict = {}
+    for name, value in options.items():
+        value_list = []


I... Have no idea what I was thinking when I wrote this... I guess I need to stop writing code when I am excessively hot. I could have sworn I re-tested this before pushing this change, but obviously not.

handle_trigger_math function to use regex, loops, and allow for multiple operations.

beauxq · 2024-10-04T12:45:14Z

Generate.py

+    weights[option_name] = new_dict
+    return weights


Not too long ago, we had a bug we needed to fix from modifying the original weights.
This looks like it doesn't need to modify the original weights (since it returns it).
Could it make a copy (deepcopy) and modify the copy and return it, leaving the original unchanged?

Fixed. Made a couple other small changes to account for the difference, including a temporary variable so the return function did not overwrite the reference in currently_targetted_weights. The difference also showed a missed piece of logic for update_weights to handle the * suffix for option names which I added as well.

filter for * name endings to update_weights, clarified weight names, and altered return value for trigger math to account for using copy instead of original.

benny-dreamly · 2024-10-05T03:44:33Z

this looks cool, gonna go more into it later

Vertraic added 2 commits October 2, 2024 01:07

Added Handler for math operations and check for math operator.

802c4f2

Updated Trigger documentation to account for these changes.

86002b0

github-actions bot added affects: core Issues/PRs that touch core and may need additional validation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Oct 2, 2024

Merge branch 'main' into MathTriggers

abfdbd6

Exempt-Medic reviewed Oct 2, 2024

View reviewed changes

Exempt-Medic added the is: enhancement Issues requesting new features or pull requests implementing new features. label Oct 2, 2024

Vertraic added 2 commits October 2, 2024 17:36

Fixed PEP errors, made code/error messages clearer,

e2ef6bb

and fixed range error and division type issues. Mostly as advised by Exempt-Medic

Merge branch 'MathTriggers' of https://github.com/Vertraic/Archipelag…

e232c8c

…o.git into MathTriggers

Vertraic requested a review from Exempt-Medic October 3, 2024 04:56

beauxq reviewed Oct 3, 2024

View reviewed changes

Simplified some code as suggested by beauxq, and replaced much of the

b1cd01a

handle_trigger_math function to use regex, loops, and allow for multiple operations.

beauxq reviewed Oct 4, 2024

View reviewed changes

Changed handle_trigger_math to use a copy of targetted weights. Added

0a24523

filter for * name endings to update_weights, clarified weight names, and altered return value for trigger math to account for using copy instead of original.

Merge branch 'main' into MathTriggers

b5fc024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Core: Added Handler for math operations and check for math operator in Triggers. #4022

Core: Added Handler for math operations and check for math operator in Triggers. #4022

Vertraic commented Oct 2, 2024 •

edited

Loading

Exempt-Medic Oct 2, 2024 •

edited

Loading

Vertraic Oct 4, 2024

Exempt-Medic Oct 2, 2024

Exempt-Medic Oct 2, 2024

Vertraic Oct 3, 2024

Exempt-Medic Oct 2, 2024

Exempt-Medic Oct 2, 2024

Exempt-Medic Oct 2, 2024

beauxq Oct 3, 2024

Vertraic Oct 4, 2024

beauxq Oct 3, 2024

beauxq Oct 3, 2024

Vertraic Oct 4, 2024

beauxq Oct 3, 2024

Vertraic Oct 4, 2024

This comment was marked as resolved.

Vertraic Oct 4, 2024

beauxq Oct 4, 2024

Vertraic Oct 4, 2024

benny-dreamly commented Oct 5, 2024

	raise Exception(f'Mathematical trigger malformed: {name} is not a string.')
	raise Exception(f"Mathematical trigger malformed: {name} is not a string.")

	raise Exception(f'Cannot perform arithmetic, non-numeric value found for : {name.split(" ")[0]}')
	raise Exception(f'Cannot perform arithmetic, non-numeric value found for: {name.split(" ")[0]}')

	raise Exception(f'Arithmetic Option out of bounds: {value_list[0]}, {value_list[2]} must < 7 digits.')
	raise Exception(f'Arithmetic Option out of bounds: {value_list[0]}, {value_list[2]} must be between -1000000 and 10000000')

	if value_list[x] > 1_000_000 or value_list[x] < -1_000_000:
	if not (-1_000_000 < value_list[x] < 1_000_000):

		@@ -378,6 +378,50 @@ def roll_linked_options(weights: dict) -> dict:
		return weights


		def handle_trigger_math(math_options: Union[dict, string], option_name: str, weights: dict):

Core: Added Handler for math operations and check for math operator in Triggers. #4022

Are you sure you want to change the base?

Core: Added Handler for math operations and check for math operator in Triggers. #4022

Conversation

Vertraic commented Oct 2, 2024 • edited Loading

What is this fixing or adding?

How was this tested?

If this makes graphical changes, please attach screenshots.

Exempt-Medic Oct 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benny-dreamly commented Oct 5, 2024

Vertraic commented Oct 2, 2024 •

edited

Loading

Exempt-Medic Oct 2, 2024 •

edited

Loading