CyberBoardPBEM / cbwindows

CyberBoard Play by EMail system for Windows
MIT License
7 stars 4 forks source link

cformRect and CBitEditView::OnImageBoardMask() and CCellForm::CreateHexMask()? #130

Open wsu-cb opened 5 months ago

wsu-cb commented 5 months ago

What is CBitEditView::OnImageBoardMask() supposed to do for cformRect boards? There is code that looks like it is intended to blacken the right and bottom edges of the tile, but, at least on my system, the tile doesn't actually change. I have verified that CB 3.10 behaves this way as well.

Also, the CCellForm::CreateHexMask() code looks like it was never intended to be called for cformRect, but it actually is called when loading a saved cformRect board. I haven't looked at this in CB 3.10 code, but it is already true in tag v3.5-prerelease-2.

DLLarson commented 5 months ago

Also, the CCellForm::CreateHexMask() code looks like it was never intended to be called for cformRect, but it actually is called when loading a saved cformRect board. I haven't looked at this in CB 3.10 code, but it is already true in tag v3.5-prerelease-2

This is the crux of it. I searched the code back to before I used GitHub and this call to CreateHexMask() is over 23 years old. I don't have any source control history available before that but I do have source code zip files of each release starting at V1.00. The flaw goes back to the beginning of CB time in archive v1.00.

It shouldn't be called for rectangular cells. It must be benign in effect since we've never discovered it before. But it's still not correct.

One great thing about code ports is that they uncover these very issues!

-Dale

wsu-cb commented 4 months ago

It shouldn't be called for rectangular cells. It must be benign in effect since we've never discovered it before. But it's still not correct.

Does this apply to both CCellForm::CreateHexMask() and CBitEditView::OnImageBoardMask(), or just CCellForm::CreateHexMask()?

DLLarson commented 4 months ago

CCellForm::CreateHexMask() shouldn't be called for anything non-hex-like. I think the method is fine as it is.

CBitEditView::OnImageBoardMask() should be fine if CreateHexMask() isn't called to create a bitmask in the first place. Then the call to pcf.GetMask() would return NULL.

....or am I missing something else?

-Dale

wsu-cb commented 4 months ago

CCellForm::CreateHexMask() shouldn't be called for anything non-hex-like. I think the method is fine as it is.

CBitEditView::OnImageBoardMask() should be fine if CreateHexMask() isn't called to create a bitmask in the first place. Then the call to pcf.GetMask() would return NULL.

....or am I missing something else?

-Dale

When pcf.GetMask() returns NULL, CBitEditView::OnImageBoardMask() executes code that looks like it is intended to blacken the right and bottom edges of the tile, but, at least on my system, the tile doesn't actually change. I have verified that CB 3.10 behaves this way as well, i.e., the tile doesn't change.

DLLarson commented 4 months ago

When pcf.GetMask() returns NULL, CBitEditView::OnImageBoardMask() executes code that looks like it is intended to blacken the right and bottom edges of the tile, but, at least on my system, the tile doesn't actually change. I have verified that CB 3.10 behaves this way as well, i.e., the tile doesn't change.

Yes. And I'm sure I had a good reason to do it. It probably extended back to my first gamebox: Tactics 2 since it had square cells. But I don't remember why.

If you apply a cell mask to a tile that is larger than the board cell that was used to create the mask, you will end up with two lines that delineate the location and area what would be displayed when placing the tile in that boards cell layer. It's an editing tool to know what will be actually rendered.

image

I just don't have a use case for this but that doesn't mean else wouldn't find a use.

I've attached a corrective patch that should clean this code up some.

0001-Fix-improper-call-to-CreateHexMask-when-shapes-are-r.patch

-Dale

wsu-cb commented 4 months ago

Have you tried adding a cell mask from a rectangular board to an oversized tile? I don't see any black on the tile after applying a mask in that case, neither in CB 3.10 nor after applying the patch in the previous msg.

DLLarson commented 4 months ago

Have you tried adding a cell mask from a rectangular board to an oversized tile? I don't see any black on the tile after applying a mask in that case,

I just did it here with the patched code:

image

I created a new tile several pixels larger than the board's cell (I just selected the board's cell size and edited the provided values to be larger.) Then I applied the board mask and I got the two lines delineating the visible area shown when dropped on the cell layer.

neither in CB 3.10 nor after applying the patch in the previous msg.

It won't work in CB 3.10 because that code doesn't have the patch so it's rendering a mask that is the same size as the tile area. It has no black areas. So it's basically a no-op.

I also noticed a bug in the eyedropper tool when I made the example. When I clicked a pixel the proper sampled color is shown but it is quickly overwritten with another color (in my case brown) upon mouse button release. This bug also exists in the current wide release. If you don't see it then it must be another problem triggered by High-DPI support in Windows.

-Dale

wsu-cb commented 4 months ago

I created a new tile several pixels larger than the board's cell (I just selected the board's cell size and edited the provided values to be larger.) Then I applied the board mask and I got the two lines delineating the visible area shown when dropped on the cell layer.

neither in CB 3.10 nor after applying the patch in the previous msg.

It won't work in CB 3.10 because that code doesn't have the patch so it's rendering a mask that is the same size as the tile area. It has no black areas. So it's basically a no-op.

So that explains why I couldn't get CB 3.10 to do it, and I have retried this after applying the patch, and this time it worked.

I also noticed a bug in the eyedropper tool when I made the example. When I clicked a pixel the proper sampled color is shown but it is quickly overwritten with another color (in my case brown) upon mouse button release. This bug also exists in the current wide release. If you don't see it then it must be another problem triggered by High-DPI support in Windows.

I had already found and fixed this while working on the wx conversion. The problem was that the ButtonUp handler conversion of the mouse msg point to the bitmap point was done incorrectly, so the color of the wrong pixel was picked up.

DLLarson commented 4 months ago

I had already found and fixed this while working on the wx conversion. The problem was that the ButtonUp handler conversion of the mouse msg point to the bitmap point was done incorrectly, so the color of the wrong pixel was picked up.

So I'm assuming that fix is coming with your next batch of porting mods?

-Dale

wsu-cb commented 4 months ago

I had already found and fixed this while working on the wx conversion. The problem was that the ButtonUp handler conversion of the mouse msg point to the bitmap point was done incorrectly, so the color of the wrong pixel was picked up.

So I'm assuming that fix is coming with your next batch of porting mods?

Yes, the PR I just posted includes a fix for this as part of the wx conversion, but not as a separate commit since I only noticed the problem when testing the converted code.