search

byWulf / threejs-dice

Javascript library to create throwable dice with threejs and cannonjs
MIT License
180 stars 69 forks source link

D4 does not display faces correctly #6

Open tim-duncan opened 6 years ago

tim-duncan commented 6 years ago

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: image

MatrixFr commented 6 years ago

+1

byWulf commented 6 years ago

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 commented 6 years ago

was this fixed?

jaxx1rr commented 5 years ago

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 commented 5 years ago

any news about this?

liffiton commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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

byWulf commented 3 years ago

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