Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wsu-cb opened this issue Jun 21, 2024 · 10 comments
Assignees
Labels
question Further information is requested

Comments

@wsu-cb
Copy link
Contributor

wsu-cb commented Jun 21, 2024

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.

@wsu-cb wsu-cb added the question Further information is requested label Jun 21, 2024
@DLLarson
Copy link
Member

DLLarson commented Jun 21, 2024

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
Copy link
Contributor Author

wsu-cb commented Jun 30, 2024

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
Copy link
Member

DLLarson commented Jul 1, 2024

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
Copy link
Contributor Author

wsu-cb commented Jul 2, 2024

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
Copy link
Member

DLLarson commented Jul 2, 2024

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
Copy link
Contributor Author

wsu-cb commented Jul 5, 2024

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
Copy link
Member

DLLarson commented Jul 6, 2024

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
Copy link
Contributor Author

wsu-cb commented Jul 7, 2024

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
Copy link
Member

DLLarson commented Jul 7, 2024

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
Copy link
Contributor Author

wsu-cb commented Jul 7, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants