-
-
Notifications
You must be signed in to change notification settings - Fork 61
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
Conversation
* 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
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 For reference, the format for the ; 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. |
I'll go with backporting the v0.2.0 system, now I know what that looks like |
Hopefully this should be compatible? |
There was a problem hiding this 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 👍
There was a problem hiding this 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 👍
* 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
Ah, thank you |
No, thank YOU! 😁 |
Changes
restriction is arbitrary.)
Fixes/Solves
What does this PR fix or solve? If applicable, put the issue number here.
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)
Names (as an example)
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 topreviously released versions, then the commits will be cherry-picked afterwards.
Done.