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

(WIP) Advanced color selector #1370

Closed

Conversation

caryoscelus
Copy link
Contributor

@caryoscelus caryoscelus commented Aug 3, 2017

I'm currently developing an advanced color selector (based on https://github.com/mbasaglia/Qt-Color-Widgets and so usable for any Qt apps, not just OpenToonz) as requested by @Agnyy. While it's far from being complete or advanced yet, i've already made it to work with OT and so have a few questions regarding integration:

  • what is the preferred way of source storage and building? As i understand, OT doesn't use many third-party dependences and uses static linking, so for now i used git submodule and add_subdirectory in CMakeLists

  • are there any special cases related to color selecting? I already run into "color 0" which is displayed as invalid style in Style Editor (though changing it through my dock works)

  • i see you're using custom icons for everything so i suppose using icons bundled with Qt-Color-Widgets would be ok?

Since my color selector widget isn't limited to OT, i'd like this thread to be kept for OT-related stuff and discuss universal features in mbasaglia/Qt-Color-Widgets#29

screenshot

Issues List:

  • Fix the color picker not picking both colors for gradients and stuff. Only lets you pick one color.
  • Fix black text in left most box when the color picker is opened for the 1st time
  • Fix Linux compile target in toonz\sources\toonz\CMakeLists.txt (see comment belot)
  • Suggestion: Relocate MI_OpenAdvancedColorSelector option in Windows Menu to after MI_OpenColorModel
  • Add MI_OpenAdvancedColorSelector to stuff\profiles\layouts\rooms\Default\menubar_template.xml to match location in Windows Menu
  • Fix styling issues (not required, but would be nice)

@MaxMixBug
Copy link

@turtleTooth Please add this to OTX

@2dvision
Copy link

2dvision commented Aug 4, 2017

Finally #791 , Thanks.

@ghost
Copy link

ghost commented Aug 5, 2017

this is really cool, not a fan of the UI yet though but that'll probably change in the future

edit: and calling it an advanced colour selector even though the original colour selector is more advanced than this new one seems a bit silly to me.

@caryoscelus
Copy link
Contributor Author

caryoscelus commented Aug 5, 2017 via email

@Agnyy
Copy link

Agnyy commented Oct 30, 2017

Test video (New) https://youtu.be/DSSkQtJxhv4

image

@2dvision
Copy link

2dvision commented Nov 7, 2017

That's perfect. Is it possible add HSV and Alpha slider too?

return;
if (palette->getStyle(styleIndex)->getMainColor() == qColorToTPixel(c))
return;
palette->setStyle(styleIndex, qColorToTPixel(c));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently any changing of color always resets brush style. You may use following code to fix:
palette->getStyle(styleIndex)->setMainColor( qColorToTPixel(c) )

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed, thx.

@adamearle
Copy link

this is just to throw a spanner in the works for people to have a think about. http://paletton.com/#uid=c0d310G3v0klllltksSs0sAeFe5f4dm

@caryoscelus
Copy link
Contributor Author

Is it possible add HSV and Alpha slider too?

HSV should be pretty trivial. As for alpha, i have plans for it, but it may need a bit of tweaking (e.g. to make it optional).

@artisteacher
Copy link
Contributor

Will the HSV and RGB sliders be optional? Does the widget fit well in a small but horizontal space? Could there be a way to create new styles in the level palette directly from the widget?

@caryoscelus
Copy link
Contributor Author

@artisteacher:

Will the HSV and RGB sliders be optional?

RGB sliders are already optional, HSV will be also when it is implemented.

Does the widget fit well in a small but horizontal space?

It doesn't fit really well into horizontally oriented space, except for "Rectangle" tab which can simply takes all space available. It should be possible to implement orientation change eventually.

Could there be a way to create new styles in the level palette directly from the widget?

Currently not possible, but should be doable in theory. One of ideas was ability to integrate any kind of palettes into the widget. Another option would be to add custom context menu on colors and add "add as new style" action there. Yet another would be to implement drag&drop

@DoomDesigner DoomDesigner mentioned this pull request Dec 1, 2017
@caryoscelus
Copy link
Contributor Author

Added HSV.

@artisteacher
Copy link
Contributor

artisteacher commented Dec 22, 2017

The functionality of the advanced color selector works great. It will pair particularly well with a workflow using the my paint brushes.

There are some areas where the UI can be refined - both in terms of blending in with the existing themes and in terms of fluidity.

  • The white border is a bit distracting since it is high contrast, not used anywhere else & has positioning issues when changing the tabs
  • The tabs should use the same styling as the style editor & palettes (i.e. no rounded corners and a darker value for the inactive tab)
  • The configure button should use an icon instead of text
  • If the selector is too narrow, you can see the tabs through the middle of the arrows which is an existing behavior with the other tabs but is more distracting here due to the configure button
  • If the selector is too narrow, the color ring placement feels out of balance (skewed a little too left)
  • If the selector is too narrow, the mode buttons can overlap the color ring
  • There should probably be a minimum height for the wheel area
  • While the black background is visually pleasing, the margin takes up unnecessary space for those of us with small screens

In addition to the existing modes, it could be helpful to have a mode that selects a set of values in a single color range.

Drag and drop for the palette colors would be fantastic, especially if you could select multiple styles at once. Even just a shortcut would work well (i.e. shift-left-click creates a new style while left-click modifies the selected style).
Thanks @caryoscelus for this great addition!

Edit: The rgb sliders have a different style from the style editor, though they fit with other sliders in the UI. The configuration settings reset upon opening OT. Also, if auto-apply is off in the style editor, returning to a previous style does not always work as expected after using the color selector.

@morevnaproject
Copy link
Contributor

We have found that with this addition it is impossible to change alpha. Here is the fix - morevnaproject-org@7fe9156

@ghost
Copy link

ghost commented Jan 3, 2018

I tried building this on Visual Studio and there are a number of errors.

@caryoscelus
Copy link
Contributor Author

Short update: alpha works with a slider, everything that i could test seems to work. I can't help with building on system i don't use without details.

Hopefully i'll answer to suggestions in more details later.

@caryoscelus caryoscelus changed the title [WIP] Advanced color selector Advanced color selector Jun 17, 2018
@711GM

This comment has been minimized.

@711GM

This comment has been minimized.

@jpturcotte

This comment has been minimized.

@ghost

This comment has been minimized.

@ghost
Copy link

ghost commented Aug 19, 2018

@caryoscelus It looks like this is failing on Windows due to a missing CMakeLists.txt file in the Qt-Color-Widgets directory

@caryoscelus
Copy link
Contributor Author

caryoscelus commented Aug 19, 2018 via email

@RodneyBaker

This comment has been minimized.

@manongjohn
Copy link
Collaborator

Looks to be some sort of configuration issue that deals with submodules that only @shun-iwasawa would likely have the ability to change?

@Agnyy
Copy link

Agnyy commented Nov 24, 2018

What needs to be done to make this PR accept?

@shun-iwasawa
Copy link
Member

shun-iwasawa commented Nov 26, 2018

@caryoscelus @Agnyy
Sorry for my late response.

Here are my thoughts:

  • Please try adding git submodule update --init --recursive to the install script in opentoonz/appveyor.yml in order to advance the Appveyor build.
  • I managed to build the PR on Windows but it was not yet straightforward:
    • Needed to call git submodule update which should be added to the document
      opentoonz/doc/how_to_build_win.md .
    • CMake adds 5 projects ( ColorWidgetsPlugin , ColorWidgetsPlugin_install , ColorWidgets-qt5 , gallery , and screenshot_bin ) only for this widget. I think it should be minimized for simplification.
    • As @turtleTooth pointed, there were a number of errors appeared. To resolve errors I did the following;
      • Removed screenshot_bin project which seems to be unnecessary.
      • Added QTCOLORWIDGETS_LIBRARY to the preprocessor definition of ColorWidgets-qt5 project.
  • Can you please cope with the above? Or I personally think it would be somewhat reasonable to simply make your library pre-built and put in thirdparty folder instead of using submodule.
  • The license notice seems to be needed to the stuff folder opentoonz/stuff/doc/LICENSE/ as the original source is under LGPL.

The widget looks neat and useful. Thank you for your contribution!

@manongjohn
Copy link
Collaborator

manongjohn commented Feb 5, 2020

Conflicts, not withstanding, there were 3 issues that I mentioned in prior comments that needed fixing.

@manongjohn
Copy link
Collaborator

Not sure what is going on with this PR, whether this will ever see the light of day, be abandoned completely or abandoned but integrated into the existing color selector.

At any rate, if this is to resume in some way, I've added an Issues Checklist to the body of the PR for stuff that was brought up. At minimum, the 1st 3 should be fixed. The 2 after that are quick changes I would like to see included. Last one can be done in a later PR.

@caryoscelus
Copy link
Contributor Author

Well, i can hardly find enough time to fix things properly, but i did fix the most urgent stuff. "Gradient editing" (for the lack of better term) is working though unuserfriendly, just as a PoC for now. Still have to merge to latest master and fix the ui

@manongjohn
Copy link
Collaborator

"Gradient editing" (for the lack of better term) is working though unuserfriendly, just as a PoC for now.

Not quite sure what you mean here. I tried the artifact and it's still only updating 1 color box. When switching to the second color, it keeps changing the same box (1st box).

I saw you changed the position of the Advanced Color Selector in menubar.cpp. Thanks. Need to update it in stuff\profiles\layouts\rooms\Default\menubar_template.xml also for it to be picked up on Windows machines.

@caryoscelus
Copy link
Contributor Author

Not quite sure what you mean here. I tried the artifact and it's still only updating 1 color box. When switching to the second color, it keeps changing the same box (1st box).

When i was testing, i didn't do it on gradients, but on whatever two-color style i found working; and it worked for me, updated the second box after switching. If you would find time for that, a video record of how things went wrong might be helpful

I saw you changed the position of the Advanced Color Selector in menubar.cpp. Thanks. Need to update it in stuff\profiles\layouts\rooms\Default\menubar_template.xml also for it to be picked up on Windows machines.

Ok, thx, will do

@@ -307,6 +307,7 @@
<separator/>
<command>MI_OpenFileBrowser</command>
<command>MI_OpenFileBrowser2</command>
<command>MI_OpenAdvancedColorSelector</command>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To match the location of if in code, please move this up below MI_OpenColorModel

@caryoscelus
Copy link
Contributor Author

@manongjohn i've fixed both positions, in .cpp and .xml

@RodneyBaker
Copy link
Collaborator

RodneyBaker commented Oct 13, 2020

The current consensus appears to be that this PR will not be added to Opentoonz.
If this is the case we (collectively) should identify (or reiterate) the reasons why this is to be the case.

For those desiring to work with the Advanced Color Selector it can be found in the Morevna release as well as via the appveyor build here:

https://ci.appveyor.com/api/buildjobs/twfy7xoy15wq5ly6/artifacts/toonz%2Fx64%2FOpenToonz.zip

Outlook for adding this Advanced Color Selector to various forks is estimated as the following:

Opentoonz (master): Not currently planned for v1.5 and unlikely thereafter without major changes
OTX: Unlikely (although some releases do include the feature)
Tahoma: Unlikely (use of similar feature set more likely)
Morevna: (as primary sponsor of the feature) addition and maintenance of the feature is likely

All interested parties are encouraged to provide feedback and status regarding decisions in their specific areas of interest (i.e. forks, additional or competing developments, etc.) As @caryoscelus has put a lot of work into the Advanced Color Selector it will be good to keep him well informed of plans moving forward.

Above all, @caryoscelus we want to keep you in the loop.
As always, thanks for all you do on behalf of the Opentoonz community.

P.S. I don't think there is any issue with having this PR continue as an ongoing WIP.

@RodneyBaker
Copy link
Collaborator

Closing this for now pending renewed interest.

2020 UI Scrub automation moved this from In Progress to Completed Dec 21, 2020
@Agnyy
Copy link

Agnyy commented Jan 22, 2021

Hello! Why did they decide to reject this request? It took a long time and it will be very sad if it is thrown out like that.

@RodneyBaker
Copy link
Collaborator

Why did they decide to reject this request?

The primary reason appears to be loss of interest in merging this feature into Opentoonz and its various forks from all parties with last update of code toward resolution of various recommendations in Sep 2020.

Your post is the first signal of interest since that time.

@Agnyy
Copy link

Agnyy commented Jan 22, 2021

What could I do to get this request accepted? This request was made not so much for me as for the artists. This system is specially designed for painting.

@manongjohn
Copy link
Collaborator

I think there was still 1 outstanding issue with Color Gradients not picking the different colors that never got resolved. It only updated the 1st color.

@ghost
Copy link

ghost commented Jan 22, 2021

I'm going to throw in my thoughts. . .
I think this feature adds even more complexity to an already complex program. Having two different panels that essentially do the same thing doesn't make a lot of sense to me. If there are issues with the main color selector, I think it makes much more sense to fix/enhance that one rather than add an additional one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
2020 PR Scrub
  
In progress
2020 UI Scrub
  
Completed
Development

Successfully merging this pull request may close these issues.

None yet