Gerold55 / laptop

Introducing the MineTest Laptop Mod
Other
26 stars 14 forks source link

TNT Sweeper Bug Fix #90

Closed Grizzly-Adam closed 6 years ago

Grizzly-Adam commented 6 years ago

Fixes bug where clearing a non-bomb marked square doesn't lower bomb count. I kept ending small easy game with 12/10 bombs cleared.

bell07 commented 6 years ago

How the issue can be reproduced? The fix should not be in this way as proposed, the code is already in the method sweeper_class:toggle_bomb_mark(sel_w, sel_h), so I need to understand why it does not work

Grizzly-Adam commented 6 years ago

Mark a square-- bomb count goes up by 1. Then clear that marked square. If that square didn't have a bomb, the bomb count does not go back down.

bell07 commented 6 years ago

I see the issue. Now I did some rework in https://github.com/bell07/minetest-laptop/commit/f91354d84eba85def9485b6157712c8980279d55. More then needed to fix this issue, but I was in flow ;-) My proposal is to unmark the bomb instead of reveal if marked. Pls check if my correction is ok for you

Grizzly-Adam commented 6 years ago

I dont know. If im in reveal mode i think it should reveal. My fix did what the user would expect.

bell07 commented 6 years ago

Hm, I tried "the original" in Win10 version: an marked field cannot be revealed, but it is not unmarked if I try to unmark them. It does just nothing. The mark is a protection to prevent the bomb revealing in original, but we do not need to follow the original.

@Grizzly-Adam, @Gerold55, All 3 ways are possible to implement, but which way we need to go? It is not neccessary to follow "the original"

Grizzly-Adam commented 6 years ago

Lets go with the do nothing the original uses-- just in case the user forgot to change modes.

bell07 commented 6 years ago

I tried it (my version +

                local sel = sweeper:get(sel_w, sel_h)
                if data.mark_mode then
                    sweeper:toggle_bomb_mark(sel)
                elseif not sel.bomb_marked then --reveal only if not marked as bomb
                    sweeper:reveal(sel_w, sel_h)
                end

Feels wrong because no visual feedback if clicked on marked bomb. So the original behavior is bad for TNTSweeper ...

Grizzly-Adam commented 6 years ago

Then I like my version better, and you like yours. Its your program.

bell07 commented 6 years ago

@Gerold55, what do you think? Reveal-Klick to marked field does: A: nothing B: unmark. Field can be revealed on second click C: unmark and reveal in one step

bell07 commented 6 years ago

Ok, for me it does not really a difference. So the next proposal is to use my implementation https://github.com/Gerold55/minetest-laptop/compare/master...bell07:pr_fix_tntsweeper for your logic "C: unmark and reveal in one step" ;-)

Grizzly-Adam commented 6 years ago

Sounds good. I am on road now taking wife in for minor surgery. Later today ai can go back to testing stuff as I will have time off work to watch her post-op.

bell07 commented 6 years ago

Replaced by #97