search

NPBruce / valkyrie

Valkyrie GM for Fantasy Flight Board Games
Apache License 2.0
506 stars 105 forks source link

Test variable: add OR condition #934

Closed antontimmermans closed 5 years ago

antontimmermans commented 6 years ago

Description of Problem or Question

Following request is an enhancement request for creating scenarios in Valkyrie.

For my new scenario I am thinking about one story with different board layouts (selected random at start) and a random selected correct outcome. To complicated for me to just start building in Valkyrie, so I actually started designing it first on paper before I put it in Valkyrie.... Crazy world eh ;-)? But while making the design I found out that programming the scenario could be less complicated if the variable test in an event have the option to add an OR condition.

Currently you can test multiple variables but they are interpreted as: "Condition1 == TRUE" AND "round == 3".. then process the actions of the event. If I want also another combination of conditions to trigger the same actions I have to create a new event with the same actions but different test conditions. This could lead to a large number of events with same actions but different conditions.

I know this could be solved with "silent" tests events all calling the same action event for more consistent behavior and less chance of programming bugs. But it would be nice if there was an option to combine this in one event (for ease of programming, testing and maintenance).

A typical use-case could be that I only want the actions to happen in certain rounds, depending if other conditions are met: IF ("tileXrevealed == TRUE" AND "Hint1Discovered == TRUE" AND " Round == 2" ) OR ("tileYrevealed == TRUE" AND "Hint1Discovered == TRUE" AND " Round == 4" ) OR ("tileYrevealed == TRUE" AND "Hint1Discovered == TRUE" AND " Round == 7" ) THEN Action: Spawn something

So yes, there is a workaround but with this option some scenarios could be written less in a complicated way.

antontimmermans commented 5 years ago

I used the latest test version that I got from @BenRQ and ALL tests in event.ini disappeared :(

BenRQ commented 5 years ago

'Conditions' is now called 'eventtests'

The old conditions should automatically be translated in the new format.

Do you confirm tests are still working?

Le sam. 24 nov. 2018 19:54, Anton notifications@github.com a écrit :

I used the latest test version that I got from @BenRQ https://github.com/BenRQ and ALL tests in event.ini disappeared :(

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/NPBruce/valkyrie/issues/934#issuecomment-441388273, or mute the thread https://github.com/notifications/unsubscribe-auth/Am56XTTEirCs36_vwtjH95q4QUHsygmMks5uyZXmgaJpZM4X8FxJ .

antontimmermans commented 5 years ago

Not sure what happened. I might have loaded the new scenario in Valkyrie 2.1.3. and later again in the alst test version that i received from you. ALL varialble tests, including the ones i created in the test version where gone. including this one: (had to make 2 screenshots to capture it in total. patronofficetest1 patronofficetest2

I am not sure but i think it might have gone wrong due to mixing old an new versions. I will stick to the test version now to avaoid this loss again.

BenRQ commented 5 years ago

I have just tested in my latest dev version, and it is working fine.

If you want a reference version, please use the one attached here, and delete the older tests versions. https://we.tl/t-gp9trTNOaU

antontimmermans commented 5 years ago

@BenRQ I am still using your lastest test version to further develop my scenario using the OR test: 2.2.2018 1125_technical. So far in the evluation of the OR condition it seems to work very well. I have not found an bug in that part (yet).

  1. In the editor it still can go wrong: If you delete a ')' it does not always delete a '('. If you then try to delete all '('and ')' there will be a solitary '(' left. It is impossible to delete this last ')' using the red '-' button. I have to edit the events.ini file to correct it again. I think you can best solve this by allowing only a delete on the '('. So the ')' do not have a red '-' button. And if you delete a '(' also delete always the first next ')' that you encounter. The behavior is then predictable and the scenario writer will have to move the remaining '( )' to the correct position anyway.

  2. The latest official release 2.1.5 doesn't support this OR test function (and will delete all tests in scenario's that use the OR test). So I am a bit stuck with having to keep using this technical version. When do you think the OR function can be releases in an official release?

BenRQ commented 5 years ago

Hi @antontimmermans, this feature is part of release 2.2. Main reason for not integrating it in release 2.1.5, is that I don't want to risk any regression on a hotfix version like version 2.1.5.

If you check the current status of the release 2.2, we have done half of it more or less. So it's not coming before January or February I think (and I may be optimistic). We have two big topics in this release:

Horrific journey integration is also a topic. #868

On top of that we have all the other smaller issues. @asn-d6 aIso recently helped us for #959 and is working on other issue at the same time.

Scope of the release is open to discussion and we can scope out some of the smaller items, but I really would like to have the scenario explorer as this is a must have today IMO.

So the question is : do you plan to release you scenario before that date ?

And thanks for the bug report, I will check it :)

BenRQ commented 5 years ago

Bug is fixed, please find here a new version : https://we.tl/t-GkivJjQhsX

I have refactored this code quite a lot since last version you were using, to allow tests in any components. There should not be any regressions, but well, better be sure :)

antontimmermans commented 5 years ago

If i run my TEST AND OR scenario in 2.2.2018115_technical it works fine. ANDORTEST.zip

although the icons are messed up: image

event.ini shows the contitions like this:

[EventTEST2]
buttons=1
event1=
eventtests=VarOperation:A,==,1 TestLogicalOperator:AND TestParenthesis:( VarOperation:B,==,1 TestLogicalOperator:OR VarOperation:C,==,1 TestParenthesis:)

[EventTEST3]
buttons=1
event1=
eventtests=VarOperation:A,==,1 TestLogicalOperator:OR VarOperation:B,==,1 TestLogicalOperator:AND VarOperation:C,==,1

When I open this scenario in the latest test version you provided 2.2.2018 1209 The icons look correct again: image (BTW i am LOVING the brightness setting). but this is what is left of the test conditions :(


[EventTEST2]
buttons=1
event1=

[EventTEST3]
buttons=1
event1=

I noticed the new syntax when i created a new testcontition

[EventTEST1]
vartests=VarOperation:A,>,0 VarTestsLogicalOperator:AND VarOperation:B,>,0 VarTestsLogicalOperator:AND VarOperation:C,>,0 
buttons=1
event1=

Conclusion:

End result seems to work fine but the import and conversion of old syntax needs to improve. I am also not a big fan of the syntax becomming longer. Check out this contition test from my latest scenario (haven't replaced the syntax with the new one yet:)

eventtests=VarOperation:Manor,==,1 TestLogicalOperator:AND VarOperation:LeisureRevealed,!=,0 TestLogicalOperator:AND TestParenthesis:( TestParenthesis:( VarOperation:Story,==,2 TestLogicalOperator:AND VarOperation:#round,>=,2 TestLogicalOperator:AND VarOperation:#round,<=,3 TestParenthesis:) TestLogicalOperator:OR TestParenthesis:( TestParenthesis:( VarOperation:Story,==,1 TestLogicalOperator:OR VarOperation:Story,==,5 TestParenthesis:) TestLogicalOperator:AND TestParenthesis:( VarOperation:#round,>=,4 TestLogicalOperator:AND VarOperation:#round,<=,6 TestParenthesis:) TestParenthesis:) TestLogicalOperator:OR TestParenthesis:( VarOperation:Story,==,2 TestLogicalOperator:AND VarOperation:#round,>=,8 TestLogicalOperator:AND VarOperation:#round,<=,11 TestParenthesis:) TestParenthesis:)

Try to debug that :)

And to answer your question: "when do I plan to release my new scenario" It is not finished yet and with the randomly selected layout (out of 2 possible layouts) and te random selected storyline (1 out of 5) I am basically writing 10 scenarios in at once. Its more work then i barainged for so around Xmas i will have a bit more Time to concentrate on it. So I hope it is ready with the releasing of Valkyrie 2.2

BenRQ commented 5 years ago

The icon messed up is because of the update of the official app. Latest technical version fixed this. So no worries on that.

Regarding the import feature, I'm making sure to support the import from the official versions of Valkyrie. I can't really support all technical versions, but I'm happy to help you fix your tests if I can.

For the official version, import and conversion of tests is working good in my tests. image

For the test syntax, I can only agree with you, it's not really pretty. But it's really reliable. I wanted to be sure I would not fall in some sh**y corner case where people would call there variables "andromeda" "oricular" and I could mix it with "and" and "or" parsing. So I've added the class type name in the test. I'm happy to change this if someone has a more elegant - and still reliable - solution to code this.

Good luck for your scenario writing, can't wait to test it !

antontimmermans commented 5 years ago

You are right choosing the reliable syntax.

For playing other scenarios i will use the latest official release (2.1.5) For working on the new one i will continue to use the latest technical version (2.2.2018 1209).

I will just make sure to make some back up's of my scenario's if i mix-up the versions ;-)

does 2.1.5 use the old syntaxt?.. or the new?

old -> new:

BenRQ commented 5 years ago

2.1.5 uses the old syntax still.

New syntax will be introduced with version 2.2

antontimmermans commented 5 years ago

@BenRQ ,

FYI: this weekend I managed to complete the complex ghost placement part of my new scenario. The (sometimes complex) conditional placement events alone add up to ~80 events. Without the OR and the test-parenthesis I estimate that this would have been 4 times more. I also ran the first 'big' test to check the placement behavior and found that some placements where incorrect. These placement errors could all be traced back to my own incorrect use of AND instead of OR (or vice versa). Which means: after this weekend with I am quite confident that this new feature works also correct in more extensive tests and is also very robust (doesn't break).

Congratulations on this job well done.

BenRQ commented 5 years ago

Thank you for your help on testing this feature @antontimmermans.

And I'm happy to hear that your scenario is getting closer to being done !

antontimmermans commented 5 years ago

Hi @BenRQ

I am affraid I bumped into a bug in the editor handling the AND/OR: image

I started to add testsvariables and they where simply placed on top of eachother (no AND/OR between them) Adding parenthesis didn't change it nor did moving anything up and down.

Valkyrie2.2.20181209 test version

BenRQ commented 5 years ago

Hi @antontimmermans would you be able to reproduce the problem easily ? Any idea what could have caused it ?

It would be great if you could switch to a more recent test version too. (I hope) I will soon create a new test version with the new scenario loading mechanism. I will then give you some instructions on how to make your scenario compatible with the new test version.

antontimmermans commented 5 years ago

Hi @BenRQ I am afraid is was very much focused on making big steps in my new scenario so I quickly solved this problem by editing the .ini file. If I remember correctly, it started by just adding 3 test variables which where stacked directly on top of each other (No AND/OR was added in between). I can't remember what I did to get some AND/OR between the variables, I think that happened when I added brackets. One AND/OR was added but not others. Moving the brackets also did not add more conditions. When ever you are ready to share a new test version I will try to recreate this situation and pay more attention to what I am clicking.

antontimmermans commented 5 years ago

@BenRQ i noticed unexpected behavior in making a new test: No AND is added inbetween test variables. image I think this happens when you add a second test variable which is NEW (not selected from the list) if you add a vaiable that already exists.. it DOES add an AND image

redwolf2 commented 5 years ago

@antontimmermans Your issue was fixed and moved by @BenRQ to this issue: https://github.com/NPBruce/valkyrie/issues/1091