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

Add function to fill non alpha pixels #1244

Merged
merged 4 commits into from Aug 18, 2019

Conversation

davidlamhauge
Copy link
Contributor

@davidlamhauge davidlamhauge commented Aug 17, 2019

This PR is rather simple. It adds a function to BitmapImage, that replaces all pixels inside mBounds with a chosen color.
I think it is a good general-purpose function, and I know I can use it in one of the features I'm working on, + the bitmap-coloring feature + two more features I plan to work on.
Could it be merged right away, it would be easier for me, since I don't have to wait for a more complex PR to be accepted and merged, before the function is in the code base.

@MrStevns
Copy link
Member

MrStevns commented Aug 17, 2019

So it's basically a fill operation that preserves alpha, I'd suggest that we call it something like that.
My suggestion would be void BitmapImage::fillNonAlpha(const QRgb& color)Aside from that I see no reason not to merge it right away.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 17, 2019

I discussed it with @scribblemaniac earlier today, and we struggled to find the right name. I wanted something that cooresponded to 'setPixel', si I called it 'setAllAlteredPixels', but I can easily live with fillNonAlpha.

@MrStevns
Copy link
Member

MrStevns commented Aug 17, 2019

Could you also please change "QRgb color" to "const QRgb& color" in the declaration. There's no need to make a copy of the QRgb object, we can simply use the reference.

The difference is:
QRgb color will copy the object, while const QRgb& will use the reference from the object argument and therefore does not create a copy. We make it "const" because it's good practice to make parameters immutable whenever possible.

A better explanation:
https://stackoverflow.com/questions/2582797/why-pass-by-const-reference-instead-of-by-value

@scribblemaniac
Copy link
Member

scribblemaniac commented Aug 17, 2019

Could you also please change "QRgb color" to "const QRgb& color" in the declaration. There's no need to make a copy of the QRgb object, we can simply use the reference.

A QRgb is just a typedef for an unsigned int, so it is probably better to copy it than reference it.

@MrStevns
Copy link
Member

MrStevns commented Aug 17, 2019

Ah yes that's true, I was thinking of QColor... well the const argument still stands.

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 17, 2019

I am confused - at a higher level.
Should I change it? Or is it, as I understand it, if I use QColor, that I should use const?

@MrStevns
Copy link
Member

MrStevns commented Aug 17, 2019

I am confused - at a higher level.
Should I change it? Or is it, as I understand it, if I use QColor, that I should use const?

Disregard the QColor bit. Using QRgb is fine but it should still be const.

@Jose-Moreno
Copy link
Member

Jose-Moreno commented Aug 17, 2019

Cool. For the name if there's still doubt, how about: setOpaquePixels() ? the transparency range would be implied as opaque pixels can only be non-transparent

@davidlamhauge
Copy link
Contributor Author

davidlamhauge commented Aug 18, 2019

It may seem like a small issue, but the name is important. Some new coder, five years from now, should find the code base tidy, organised and understandable.
Right now the name is 'fillNonAlpha', which explains what it does. It is usable.
I would prefer something that corresponds to 'setPixel', that sets the QRgb on one pixel. How about calling the function 'setNonAlphaPixels'?

@MrStevns
Copy link
Member

MrStevns commented Aug 18, 2019

In that case call it fillNonAlphaPixels, I'm not fan of using "set" here since that prefix is often used to indicate that we set a member variable with the input parameter, which does not happen here.

@MrStevns MrStevns changed the title setAllAlteredPixels function added Add function to fill non alpha pixels Aug 18, 2019
@Jose-Moreno
Copy link
Member

Jose-Moreno commented Aug 18, 2019

Non-alpha == Opaque If set has a different connotation that's fine, then fillOpaquePixels would be coherent, or is it the case that nonAlpha as an identifier category is being used as convention in other methods? in such case then i guess for consistency it should be fillNonAlphaPixels 🤷‍♂️

@MrStevns
Copy link
Member

MrStevns commented Aug 18, 2019

I'm merging. If we find a bug later, we can fix it then...

@MrStevns MrStevns merged commit c79cee1 into pencil2d:master Aug 18, 2019
@chchwy chchwy added this to the 0.6.5 milestone Nov 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants