Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dice: Backport 0.2.0 face_values format (kinda) #308

Merged
merged 4 commits into from
Sep 17, 2023

Conversation

20kdc
Copy link
Contributor

@20kdc 20kdc commented Sep 7, 2023

Changes

  • face_values can now have the keys and values swapped, as in 0.2.0. Internal representation is a dictionary of values (as strings) to array of Vector3 normals. Array mustn't be empty. The first vector will be used as the rotation when setting the value.
  • Removed restriction on value count matching dice sides. (While the total amount of normals could be checked, IMO this
    restriction is arbitrary.)
  • Face values are arbitrary; they are now stringified during load. Numbers are parsed when totalling requires it.
  • When only a single dice is selected, the Total indicator instead now says Value, and the value given is "raw" (i.e. for named faces, this shows the face name).

Fixes/Solves
What does this PR fix or solve? If applicable, put the issue number here.

image
This screenshot is an example use-case: Fudge/FATE dice. These have two blanks (value 0), two + (value 1) symbols and two - (value -1) symbols.
The way you calculate rolls with these is to roll four at once, then total the values. Tabletop Club almost supports this perfectly, but without this PR, their values cannot be properly represented (at least without perhaps tricks like int/float differentiation) and thus it's not possible to get a total.

Numbers (as used)

face_values = {
    Vector2(0.0, 0.0): 1,
    Vector2(180.0, 0.0): 1,
    Vector2(0.0, 90.0): 0,
    Vector2(0.0, -90.0): 0,
    Vector2(-90.0, 0.0): -1,
    Vector2(90.0, 0.0): -1
}

Names (as an example)

face_values = {
    Vector2(0.0, 0.0): "+",
    Vector2(180.0, 0.0): "+",
    Vector2(0.0, 90.0): " ",
    Vector2(0.0, -90.0): " ",
    Vector2(-90.0, 0.0): "-",
    Vector2(90.0, 0.0): "-"
}

This also mostly helps #209 (though it's not possible to specify a numeric value for a named value).

NOTE: Make sure this PR goes to the master branch! If this fix applies to
previously released versions, then the commits will be cherry-picked afterwards.

Done.

* face_values values can now be Vector2 *or* an array of Vector2. Internal representation is array of Vector3. Array mustn't be empty so that code which sets the value works and makes sense.
* Removed restriction on value count matching dice sides. (While the total amount of normals could be checked, IMO this restriction is arbitrary and only likely to cause more confusion than it solves. Branch is on the way out anyway so any change that would really break this will probably nuke everything anyway.)

Tested on a personal project; a screenshot will be included with the PR.
… one normal in the AssetDB, as it could error otherwise if the user tries to set a value
@drwhut
Copy link
Owner

drwhut commented Sep 8, 2023

Thank you for the PR! 🥳

My only concern with it's current implementation is that it would not be compatible with the new face-value system in v0.2.0 (which has already been implemented and tested in the 0.2-rewrite branch, see here for how the game reads the config.cfg file on that branch).

For reference, the format for the face_values property in v0.2.0 is essentially reversed from what it is in v0.1.x, in that the keys are the rotation required to make the given face point upwards, and the value is the, well, value:

; An example 'face_values' from v0.2.0:
face_values = {
    Vector2(0.0, 0.0): null,
    Vector2(90.0, 0.0): 2,
    Vector2(0.0, 90.0): 4.5,
    Vector2(180.0, 0.0): "wat",
}

I did it this way for two reasons: 1. it solved the duplicate face-value problem, as only the keys are required to be unique, and 2. I figured it is incredibly unlikely that anyone would want two separate values for one face.

I've also made it so that in v0.2.0, the game will still accept the current format for backwards-compatibility, but it will show a warning saying the new format should be used instead.

So yeah to prevent incompatibilites between versions, either the v0.2.0 system will need to be backported, or the v0.2.0 system will need to be replaced with this PR's system.

@20kdc
Copy link
Contributor Author

20kdc commented Sep 8, 2023

I'll go with backporting the v0.2.0 system, now I know what that looks like

@20kdc 20kdc changed the title Dice: Multiple normals per face value Dice: Backport 0.2.0 face_values format (kinda) Sep 8, 2023
@20kdc
Copy link
Contributor Author

20kdc commented Sep 8, 2023

Hopefully this should be compatible?

Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Looks good! I've just got a few points that need addressing, but once they're resolved I'll test the changes on my end and merge if I can't find anything else 👍

Copy link
Owner

@drwhut drwhut left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! Will test as soon as I can before merging 👍

@drwhut drwhut merged commit 10f637a into drwhut:master Sep 17, 2023
drwhut pushed a commit that referenced this pull request Sep 17, 2023
* Dice: Multiple normals per face value

* face_values values can now be Vector2 *or* an array of Vector2. Internal representation is array of Vector3. Array mustn't be empty so that code which sets the value works and makes sense.
* Removed restriction on value count matching dice sides. (While the total amount of normals could be checked, IMO this restriction is arbitrary and only likely to cause more confusion than it solves. Branch is on the way out anyway so any change that would really break this will probably nuke everything anyway.)

Tested on a personal project; a screenshot will be included with the PR.

* Dice: face_values: Probably should sanity-check that there's at least one normal in the AssetDB, as it could error otherwise if the user tries to set a value

* Dice: face_values: Implement 0.2.0-style

* Dice: Limit value data types to what v0.2.0 will accept, fix documentation
@20kdc
Copy link
Contributor Author

20kdc commented Sep 17, 2023

Ah, thank you

@drwhut
Copy link
Owner

drwhut commented Sep 17, 2023

No, thank YOU! 😁

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