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

Enable track-wide color coding #5573

Merged
merged 77 commits into from
Oct 20, 2020
Merged

Enable track-wide color coding #5573

merged 77 commits into from
Oct 20, 2020

Conversation

ryuukumar
Copy link
Member

@ryuukumar ryuukumar commented Jul 10, 2020

What does this PR do?

This PR adds the following features:

  • BB pattern-like color coding, except on all tracks
  • Saves the color for each track in the project
  • Resets the color of the track to the default color
  • Enables custom colors in the color dialog for 48 better-looking colors
  • Allows picking random colors from these custom colors
  • Adds a band in the track grip where the track color is displayed
  • Changes a demo in "demos" to demonstrate color-coding (in both track and mixer)

Demo

Updated (current state)

image

image

image


I believe this feature will truly lift LMMS to a whole new level, as this is something most commercial DAWs have. And it's not mandatory; without using it, LMMS still appears with the same default colors like everyone is used to.

@LmmsBot
Copy link

LmmsBot commented Jul 10, 2020

🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩

Linux

Windows

macOS

🤖
{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://9619-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-808%2Bg3615757-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9619?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://9617-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-808%2Bg361575751-mingw-win32.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9617?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://9615-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-808%2Bg361575751-mingw-win64.exe"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9615?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}, {"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/j9s30xy0rgbp27s8/artifacts/build/lmms-1.2.2-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35811650"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/p6h3vvk9cg3v590d/artifacts/build/lmms-1.2.2-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/35811650"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://9616-15778896-gh.circle-artifacts.com/0/lmms-1.2.3-808%2Bg361575751-mac10.13.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/9616?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}]}, "commit_sha": "01ffe71fc949da1faf3d0990836556a146ecfc6c"}

@ryuukumar
Copy link
Member Author

The melody pattern appears a little bit off (on the right side) compared to the rest, any idea why?

image

@Spekular
Copy link
Member

I don't think this is your PR's fault, the inner border for melody clips is drawn one pixel to the left of where it should be (at least at high zooms/track heights).

@ryuukumar
Copy link
Member Author

@Spekular yeah I've noticed this in other builds too, shall I fix it or is it not a big deal?

@ryuukumar
Copy link
Member Author

Noted bug

The background of beat+bassline tracks is also colored, this may be related to the bug where initialising colors are ignored for some reason.

image

I made the background colorful to emphasise the bug, it doesn't actually appear like this xD

@Spekular
Copy link
Member

@Spekular yeah I've noticed this in other builds too, shall I fix it or is it not a big deal?

I definitely think it should be fixed, so the question is if it's in this PR or another. I think I'd prefer a separate PR. As a pure bugfix, that one could get merged rather quickly and not be tied to however long this takes to merge. It'd also make the diff for this PR cleaner. It does make more work for you with potential merge conflicts, and I prefer a fix in this PR over no fix at all, so I leave the decision to you.

@ryuukumar
Copy link
Member Author

Noted bug

New TCOs do not take up the track color when a custom color has been assigned.

@ryuukumar
Copy link
Member Author

@Spekular yeah I think another PR would be a better choice since it's (hopefully, probably) a really tiny fix. It'll take some time setting it up but I think it would save everyone time not having to wait for this PR to merge.

@ryuukumar
Copy link
Member Author

Fixed all the bugs I found related to my changes, and a basic color coding implementation is now complete. I have also changed the styling on the sample track TCO because some colors look awkward with the old settings.

Looking for people willing to test this PR for potentially undetected bugs.

@ryuukumar
Copy link
Member Author

By the latest commit, QColorDialog's "Basic Colors" are overwritten to

  1. look good as they are sorted
  2. be relatively dark colors to not cause problems with seeing tiny notes

Demo

image

These colors are obtained algorthmically using HSL values

@ryuukumar
Copy link
Member Author

I've now added a feature where the side of the track colors itself based on the color of the entire track. This feature is purely subjective and if people take this as an unwelcome feature I'll remove it. I just think this looks good, and I'd vouch for it being implemented in its current state.

I have pushed an edit updating a demo, which I didn't realise I did. I'll replace it with the original and use another file for my testing (or we could leave it like that and color-code all the other projects to make them look better. I got the time to do that.)

Demo

image

image

Track colors were placed manually, the fading of the color as it goes down is not an implemented option

@ryuukumar
Copy link
Member Author

ryuukumar commented Jul 15, 2020

Things I still have to complete:

  • Make selections also depend on color
  • Completely migrate individual TCO colors to one master track color setting
  • Disable the gradient on the side when track is muted (or make it a different gray color)

@ryuukumar
Copy link
Member Author

ryuukumar commented Jul 15, 2020

Noted bug (fixed)

TCOs do not take up track color.

Steps to Reproduce

  1. Use a track with a set color and TCOs
  2. Reset track color
  3. Select some TCOs
  4. Set the same track color

The TCOs show with the right color only while selected.

The build is broken; I will patch it with a commit later.

@ryuukumar
Copy link
Member Author

ryuukumar commented Oct 7, 2020

@SecondFlight,

I think this will be the final version for the demo coloring, please do confirm:

image

I've colored it to make it aesthetically pleasing, not to serve any color-coding purpose (red for drums, blue for leads etc.)

If you're alright with this, I'll commit with the update.

@ryuukumar
Copy link
Member Author

ryuukumar commented Oct 8, 2020

Alright, I've updated the demo. If some changes are requested, I'll commit with another a modified copy.

@ryuukumar
Copy link
Member Author

I've added comments for anything that isn't obvious by the naming. If the current state of the PR is satisfactory, then I can finally call it a day on this.

@IanCaio
Copy link
Contributor

IanCaio commented Oct 18, 2020

@russiankumar I think you added some submodules files by accident. They were updated lately, so you might have merged master with the updated submodules without running git submodule update on the branch PR. This happened to me before, I don't remember exactly, but I think I had to go back to the commit where master was merged and then update the submodules before moving to the next commit (so the files from the submodules are not staged).

@ryuukumar
Copy link
Member Author

Oh no.... I think I messed up more than I fixed

@ryuukumar ryuukumar force-pushed the color-exp branch 3 times, most recently from 9baf164 to 75617d5 Compare October 18, 2020 04:54
@ryuukumar
Copy link
Member Author

ryuukumar commented Oct 18, 2020

Phew... seems like it's fixed. Now I need to re-add all those comments xD

Edit: Sorry for all the force pushes, I'm way too inexperienced with GitHub.

@IanCaio
Copy link
Contributor

IanCaio commented Oct 18, 2020

I think force pushing is the only way to do it, because you have to go back in history and make new commits (at least I had to do it too when it happened to me). After you stage and commit the submodule files even if you update them they are still in the change log, so you have to go back in history like you did afaik. Maybe someone more experienced with git knows if there's a better way, because I imagine it would be a big mess to have to do that after you made lots of commits over the merge that caused that issue.

Anyways, important thing is that you got it fixed 😃

@ryuukumar
Copy link
Member Author

ryuukumar commented Oct 19, 2020

I think this is done. @Spekular, if you approve the latest changes, could you merge this?

Oh, and also an out of context question: is there an automated system for developer credit (that long list of contributors), or do the PR authors have to add it in themselves?

@DomClark
Copy link
Member

The contributor list is automatically generated from git history. (At least in theory - it can be incorrect for CI builds due to shallow cloning, but we try to get it right for releases...)

@SecondFlight
Copy link
Member

SecondFlight commented Oct 20, 2020

Sorry for missing this for so long. The colors are good with me 👍

@ryuukumar
Copy link
Member Author

image

Yeah, it does mention me, but it seems it pulled my nickname out of somewhere xD

Anyways, this PR is done. Thanks for all the help everyone offered! Can't wait to see this enter mainstream LMMS :)

@superpaik
Copy link
Contributor

The color system works really fine and I think It is something that will be greatly appreciated by the community. Thanks for the effort!

I found just one issue that I don't know if it's related to this PR or not (maybe is due to QT framework)
Windows (10 at least) has a functionality for changing the screen resolution called "Scale and layout" where you don't change the screen resolution but how the apps display its content (applying a % of increment to text and UI items). It helps in laptops with high resolution screen in order to make texts readable. Long story short. When this parameter is set to, say 125%, the "Pick Screen Color" functionality that it is inside the "Select color" popup window doesn't return the color where the cursor is. The cursor position showed in the popup window shows a value like the display has a lower resolution. When is set to 100% it works fine.

@Spekular
Copy link
Member

I think this is done. @Spekular, if you approve the latest changes, could you merge this?

Yes :)

@Spekular Spekular merged commit f5d0524 into LMMS:master Oct 20, 2020
@ryuukumar
Copy link
Member Author

@superpaik thanks for the test! I believe that's a QT issue, since from what I understood it has to do with the box with the color gradient. I'll look into it and if there is a fix I can think of I'll make another PR.

Glad this one is finally merged!

@qnebra qnebra mentioned this pull request Nov 15, 2020
29 tasks
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this pull request Jun 28, 2022
* Enable track-wide color coding

* Add support for automation tracks

* Allow saving & loading track colors

* Allow track color to be reset to default

* Partially migrate common settings to Track.cpp, fix bug

* Completely migrate local TCO color functions to TCO class, fix bug

* Set QColorDialog colors to better colors

* Color the side of the track according to TCO colors

* Disable color gradient when muted

* Change selection color to depend on TCO color

* Fix breaking builds

* Bug fix

* Fix another bug where BB track colors wouldn't load

* Restore changed demo to original state

* Fix BB Editor bug

* Fix breaking builds

* Allow random color picking

* Fix copy-paste bug

* Change how color is painted on a track

* Cleanup, and implement per-pattern colors

* Cleanup comments

* Migrate some functions

* Remove redundant function

* Rename some functions

* Migrate duplicates to superclass

* Use ColorChooser and reorder some includes

* Change how colors are saved

* Fix some formatting

* Fix some code

* Change how clip colors work

* Fix some unexpected behaviors

* Fix note border coloring being green on colored tracks

* Change name of an option

* Remove redundant code

* Fix ghost changes

* Remove colorRgb

* Rename backgroundColor, remove some variables we don't use

* Remove a redundant variable

* Migrate some duplicates to superclass

* Check for nullpointer

* Remove redundant variable

* Update some logic

* Change how muted colors are displayed

* Change how dark muted tracks become

* Place setModified() in appropriate places

* Make getColorForDisplay() function

* Change how colors are organised and saved

* Remove m_useStyleColor

* Remove a comment

* Quick changes

* Change how colors are saved

* Remove redundant stuff

* Remove redundant stuff pt. 2

* Change how colors are copied

* Fixes pt. 3

* Fixes pt. 4

* Change spaces to tabs

* Fix pseudochanges

* Remove s_lastTCOColor

* Fix pseudochanges pt. 2

* Fix breaking builds

* Add files via upload

* Add comments (again)
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