D4 does not display faces correctly

[tim-duncan]

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:

[MatrixFr]

+1

[byWulf]

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! :)

[giorgiobeggiora]

was this fixed?

[jaxx1rr]

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']];

[giorgiobeggiora]

any news about this?

[liffiton]

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.

[tim-duncan]

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.

[byWulf]

Thanks for the help guys! Feel free the resolve the merge conflicts @tim-duncan and I would merge your PR :)

[byWulf]

Please try it out, I just merged @eipporko 's PR. If that is enough, I would close @tim-duncan 's PR then.