threejs-dice
threejs-dice copied to clipboard
byWulf
D4 does not display faces correctly
The 4-sided dice does not display its faces properly. The number at the top of the pyramid is often different for each face - it should be the same number specified in the value property.
To reproduce...simply switch out the D6 and replace them with D4 objects in the rolling.html example. Lines 105 & 106 become:
for (var i = 0; i < 4; i++) {
var die = new DiceD4({size: 1.5, backColor: colors[i]});
The throw should have dice displaying one each of red=1, yellow=2, green=3 & blue=4 at the top. Instead the actual throw shows red=1 and yellow=2 as expected, but the blue and green dice have mismatched numbers:
Ty for the report. I'm currently moving to a new city so time is short. I try to look into this in a few weeks. If you find the bug until I had time, feel free to submit a pull request! :)
I tried to fix it but failed misserably.. I think sometimes it works and sometimes it doesnt and thats my problem trying to modify the faces , also why is there a 0 array the other dice dont have that: this.faceTexts = [[], ['0', '0', '0'], ['2', '4', '3'], ['1', '3', '4'], ['2', '1', '4'], ['1', '2', '3']];
Feel free to try the change in my pull request. It's a few lines added to shiftUpperValue(). If anyone runs into problems with it, please let me know.
I have tested the fix by Mark (@liffiton) and found that it was almost there - see my comments on his PR #8 . Removing the second restriction in the if-condition as I suggest seems to work for all cases.
The fix probably isn't in the correct location in a design sense and should be moved back into the DiceD4 class somehow - probably there should be an override-able method in the base class for updating materials for a given face value.
Thanks for the help guys! Feel free the resolve the merge conflicts @tim-duncan and I would merge your PR :)
Please try it out, I just merged @eipporko 's PR. If that is enough, I would close @tim-duncan 's PR then.