-
Notifications
You must be signed in to change notification settings - Fork 75
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
Add support for image cropping inside ctrlImageButton #1536
base: master
Are you sure you want to change the base?
Add support for image cropping inside ctrlImageButton #1536
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor annotations. Also can you please add tests at least for the controls?
You can easily create a Mock for ITexture
(if it doesn't exist yet) and use that to check that the correct Draw-calls are done. See similar tests using the MOCK_*
macros.
Those tests should be simple enough (e.g. use "nice" sizes) that they serve also as documentation on how the feature is supposed to work.
Unfortunately I don't see an easy way to test the gl*
classes, but the changes look correct.
5f8fc67
to
e16f5f6
Compare
all things fixed. I'll add unit tests across following days. |
Thanks, looks good! If you need help with the tests feel free to ask. |
e16f5f6
to
d49c333
Compare
I've added some tests as well as corrected inverseLerp to not be templated. Let me know it this suffices, or if you want some more elaborate testing. |
9aaabba
to
dbd5a40
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A bit on improvements of the tests while we are at it. Especially we could use a test for the drawPercent which has been missing for to long.
You could create mock functions for the OpenGL functions like glVertexPointer
and either redirect them to some MOCK_FUNCTION(...)
mocks or to some handwritten ones storing the last passed values. See
RTTR_STUB_FUNCTION(glGenTextures, rttrOglMock2::glGenTextures); |
tests/s25Main/UI/testImageButton.cpp
Outdated
TestTexture(Extent size) : size_(size) {} | ||
Position GetOrigin() const override | ||
{ | ||
return Position::all(0); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As your new methods use the origin I'd say it should be tested too that it is used correctly, i.e. create (at least) 2 TestTextures with different sizes and/or origins.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will do. I've started doing it and found some problems. I don't quite get the idea of image origin - I assume this should generally work so that if I set origin to (5,8) and draw image somewhere in the game at (30,40), then at this position the pixel from the (5,8) is drawn and the image's top left corner starts at (25,32).
This is problematic when drawing image buttons - my assumption is that they should always be centered in the buttons, regardless of the origin values. Should this be the case or am I getting something wrong?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes IIRC the origin is mostly used for the textures of buildings etc where the origin may vary depending on the nation etc. I don't think it is useful for button textures, which I mentioned in https://github.com/Return-To-The-Roots/s25client/pull/1534/files#r973562992
So yes I do share your opinion that the image should always be centred and the origin ignored for buttons.
Maybe @Spikeone has something to add.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a slight brain meltdown because unit tests returned for me different position values passed to img.Draw()
when specifying different origins, but then I remembered that the origin thingy is resolved inside that function ( https://github.com/Return-To-The-Roots/s25client/blob/master/libs/s25main/ogl/glArchivItem_Bitmap.cpp#L45 ) so I think it has to be that way. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, that means that the button control MUST not use the origin when calling Draw
as that is handled by Draw
.
Or actually if Draw
handles origin
then in order to have the image always centered the button control must add the origin to account for it being removed in the draw call. So the current change at https://github.com/Return-To-The-Roots/s25client/pull/1536/files#diff-adfa4d0f1a6924716e377c3f74e76a930efc15607563954cee41631febf87c4fR14 would make the image being offset by 2 times the origin.
I guess current button textures don't have an origin set so this wasn't an issue before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I you look closely then GetImageRect() creates rect with position = -origin
so the calculations are correct there, because my subtraction effectively adds ;) I guess that's confusing though, so I'll look into getting origin directly and add it there instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That would make a great end-to-end test: Specify a non-zero origin for an image used in a button and then check that the actual draw call (i.e. the OpenGL coords) will draw the image at the exact coordinates. I.e. that you modification and the origin handling in the draw call actually do cancel each other out as you describe here (I checked: you are right)
And maybe add a short comment to the line removing the origin that this "compensates" (or any better word) the offset so the image is drawn exactly where specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've updated the comment, doing the end-to-end test is yet to follow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes IIRC the origin is mostly used for the textures of buildings etc where the origin may vary depending on the nation etc. I don't think it is useful for button textures, which I mentioned in https://github.com/Return-To-The-Roots/s25client/pull/1534/files#r973562992 So yes I do share your opinion that the image should always be centred and the origin ignored for buttons.
Maybe @Spikeone has something to add.
Only if we plan to add buttons with text and image - but currently I don't think we'd need that in any of the current interfaces.
// nothing to draw? | ||
if(!percent) | ||
return; | ||
RTTR_Assert(percent <= 100); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be tested too. Especially coming up with a few corner cases. I.e. at least percent = 0 or 100 and some values where you notice the (correct) usage of floor/ceil below. I.e. choose appropriate image size and percent values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added some unit test here, but I'm not quite sure on how I should approach the float/ceil thingy - I can't really subclass glSmartBitmap
and mock drawRect
unless I make it virtual, which I think shouldn't be done just for unit tests. The texture coords don't follow image size in pixels, for example I tried to create 50x100 bitmap and coord span for the texture was something like 0 .. 0.7825. Checking for magic float values seems not to be the best approach either.
oops, I've got some comments to the code in "pending" status and they were not there for you to see. I'll try to resolve stuff you've mentioned by tomorrow. |
dbd5a40
to
c323eef
Compare
c323eef
to
783cb2a
Compare
No description provided.