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 color picker for setting transparent color of a tileset #1173

Closed
bjorn opened this issue Jan 9, 2016 · 12 comments
Closed

Add color picker for setting transparent color of a tileset #1173

bjorn opened this issue Jan 9, 2016 · 12 comments
Labels
feature It's a feature, not a bug.

Comments

@bjorn
Copy link
Member

bjorn commented Jan 9, 2016

It would be helpful, if the transparent color of a tileset could be chosen by using a color picker tool on the tileset image.

@bjorn bjorn added the feature It's a feature, not a bug. label Jan 9, 2016
@mrjnumber1
Copy link

i know you said this wasn't high priority, but i hope you'll reconsider. sorry i have nothing to add, just want to say it's a convenient feature to have :)

@bjorn bjorn added the Urgent label Jun 12, 2016
@bjorn bjorn added the Bounty label Jul 10, 2016
@bjorn
Copy link
Member Author

bjorn commented Jul 21, 2016

Thanks @Alturos for starting to work on this! Please feel free to ask any questions you may have.

The way I imagined to implement this, would be to add a color picking button, which would cause a pop-up widget to open where the image is loaded and displayed. Left-clicking on it should pick the color of clicked pixel and clicking outside or right-clicking should close it again. Of course, if you have other ideas feel free to suggest or try them.

@Alturos
Copy link
Contributor

Alturos commented Jul 21, 2016

That's actually very close to what my plan was. I was effectively going to take a similar route as the QColourDialog.

  • Add a button next to the pick a colour button
  • Pop up a window with the image in it
  • Mouseover updates a little area with the colour preview, clicking seals it.
  • Hit Ok to accept, or cancel to not.

That feels a little more consistent with the general UI theme, but happy to slant it more toward the quicker-to-done path you outline above.

@bjorn
Copy link
Member Author

bjorn commented Jul 21, 2016

I like the idea of a little preview area (and possibly a display of the hex color value next to it). But indeed, I think a single click should be enough rather than adding OK / Cancel buttons. I would try the Qt::Popup window flag to avoid creating a draggable window.

@Alturos
Copy link
Contributor

Alturos commented Jul 21, 2016

Alright, I can go with that. One question next is on window size, do we allow unlimited window size, add a scrollable area, resizable window?

My other question is on styling and formatting, do you have any guidelines published on that? So far I'm just sort of making things looks the same as they do, but there are things I'm curious about, like auto for example.

@bjorn
Copy link
Member Author

bjorn commented Jul 21, 2016

One question next is on window size, do we allow unlimited window size, add a scrollable area, resizable window?

I think just having a reasonable maximum (maybe based on desktop size) would be enough. Beyond that the image would be rendered scaled down. Of course one could get fancy and allow zooming/panning, but I think it's rarely needed.

My other question is on styling and formatting, do you have any guidelines published on that? So far I'm just sort of making things looks the same as they do, but there are things I'm curious about, like auto for example.

I'm pretty much using the Qt coding style, but there are some differences. Making things look like the existing code should be fine.

Use auto when it improves readability. For example, I would use it for iterators or when the type is clear, like when assigning from new or some cast, but not to replace anything shorter than 5 characters.

@Alturos
Copy link
Contributor

Alturos commented Jul 21, 2016

My only concern with scaling is the ease of selecting the colour from a pixel when scaled (usability and potential added complexity in coding).

@bjorn
Copy link
Member Author

bjorn commented Jul 21, 2016

My only concern with scaling is the ease of selecting the colour from a pixel when scaled (usability and potential added complexity in coding).

There would not be complexity because you'd just divide by the scaling factor and pick from the source image. For ease of selecting, I think it will be very rare to have only tiny areas of the transparent color in a tileset, which would be the only case where you'd run into issues.

However, we could also do some kind of auto-panning approach instead of scaling. Where once the maximum window size is reached, the image is shifted proportionally to the location of the mouse on the window. Of course if you need to click a single pixel, that could still lead to issues, but it could make it easier to click the right place in general.

@Alturos
Copy link
Contributor

Alturos commented Jul 21, 2016

I'll play around with some different ideas then, see which ends up "Feeling" right. The "Most" case is the easy one, but I see no reason to sacrifice the special cases if it doesn't make it more difficult to use.

@Alturos
Copy link
Contributor

Alturos commented Jul 24, 2016

Brief update, I have the changes for this functionally complete. Going to spend some more time testing/verifying and cleaning up the UI/UX around it.

@bjorn
Copy link
Member Author

bjorn commented Jul 25, 2016

@Alturos Nice to hear! So I think it should be possible to still include this feature in the Tiled 0.17 release. I'd like to announce string freeze for this release as soon as possible.

@Alturos
Copy link
Contributor

Alturos commented Jul 26, 2016

At this stage, I'm pretty happy with the UI and functionality, just need to do some cleanup to get it in line with the code style and I'll get the pull request together. Might be a couple days, but shouldn't be more than that.

Alturos added a commit to Alturos/tiled that referenced this issue Jul 29, 2016
Alturos added a commit to Alturos/tiled that referenced this issue Jul 29, 2016
Alturos added a commit to Alturos/tiled that referenced this issue Jul 29, 2016
Alturos added a commit to Alturos/tiled that referenced this issue Aug 2, 2016
Alturos added a commit to Alturos/tiled that referenced this issue Aug 7, 2016
Alturos added a commit to Alturos/tiled that referenced this issue Aug 7, 2016
Alturos added a commit to Alturos/tiled that referenced this issue Aug 7, 2016
@bjorn bjorn closed this as completed in 05ef85b Aug 7, 2016
bjorn added a commit that referenced this issue Aug 7, 2016
* Dropped "Tileset Image" title (it could also display the scale, but
  I think this information is not important)

* Replaced the "Preview" text with the color name (hex format)

* Fixed small layout glitch first time the preview icon was set

* Fixed screen on which the popup appears after moving the New Tileset
  dialog to another screen (by re-creating the popup each time it is
  needed)

* Added frames around image and color preview

* Don't set the color picker button to enabled when no image has been
  specified

Issue #1173
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature It's a feature, not a bug.
Projects
None yet
Development

No branches or pull requests

3 participants