-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Color scheme for spectrogram #830
Conversation
Added translatable strings for crash/error reporting and the new spectrogram colour preferences (#830)
We're still interested in this proposal. Please see more discussion at the Audacity Forum. |
@Paul-Licameli TBH, I'm not fully satisfied with CLA. I just wish Audacity would remain free, open and be much greater than now. 😀 |
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'll complete this first review for now, but I need to examine AColor.cpp more closely, and consider whether there might be another fix for the sync lock tiles.
@@ -137,7 +136,6 @@ class SetTrackVisualsCommand : public SetTrackBase | |||
|
|||
bool bHasUseSpecPrefs; | |||
bool bHasSpectralSelect; | |||
bool bHasGrayScale; |
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.
For this command, you take away the checkbox for grayscale, because there is now a more-than-binary choice.
You should put back a a choice control so that this command can still set the visuals.
See examples of the use of TieChocie in the src/commands directory for how to do it.
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.
Note that this change will also break any macro scripts in existence that happen to use this checkbox parameter.
But I think that is unimportant. Do nothing about that.
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.
Added 'Spectro. Color scheme' to 'Set Track Visuals' macro command.
src/prefs/SpectrogramSettings.h
Outdated
@@ -128,7 +129,17 @@ class AUDACITY_DLL_API SpectrogramSettings : public PrefsListener | |||
size_t GetFFTLength() const; // window size (times zero padding, if STFT) | |||
size_t NBins() const; | |||
|
|||
bool isGrayscale; | |||
typedef int ColorScheme; |
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.
In C++11 and later, it's preferable not to use typedef but instead:
using ColorScheme = int;
However -- I'd rather not use this line at all...
src/prefs/SpectrogramSettings.h
Outdated
@@ -128,7 +129,17 @@ class AUDACITY_DLL_API SpectrogramSettings : public PrefsListener | |||
size_t GetFFTLength() const; // window size (times zero padding, if STFT) | |||
size_t NBins() const; | |||
|
|||
bool isGrayscale; | |||
typedef int ColorScheme; | |||
enum ColorSchemeValues : int { |
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.
... and instead write
enum ColorScheme : unsigned {
I find it preferable to make static type distinctions where you can to aid compile time checking.
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.
The change you suggested breaks template type deduction and cause compile errors. (At least at VS2017)
I won't apply this.
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 ignore that suggestion
|
||
void AColor::PreComputeGradient() { | ||
if (gradient_inited) return; | ||
gradient_inited = 1; | ||
|
||
// Keep in correspondence with enum SpectrogramSettings::ColorScheme |
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.
You should say that somehow with a compile time assertion
static_assert(SpectrogramSettings::csNumColorScheme == colorSchemes, "Broken correspondence ");
But don't put that line in this file. Avoid another #include and instead put in in SpectrogramSettings.cpp
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.
Added the static_assert in SpectrogramSettings.cpp, along with wxASSERT(csNumColorScheme == result.size());
.
I coudln't find better place to put it.
static const int gradientSteps = 512; | ||
static unsigned char gradient_pre[ColorGradientTotal][2][gradientSteps][3]; | ||
static const int colorSchemes = 4; | ||
static const int gradientSteps = 256; |
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.
Why changed from 512?
I'm not saying it's 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.
To save memory and gain some speed. It might be trivial.
Anyway, this does not affect visual quality.
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 leave it so
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.
Applied with preference migration 😉
@@ -254,7 +272,7 @@ void SpectrogramSettings::LoadPrefs() | |||
|
|||
gPrefs->Read(wxT("/Spectrum/WindowType"), &windowType, eWinFuncHann); | |||
|
|||
isGrayscale = (gPrefs->Read(wxT("/Spectrum/Grayscale"), 0L) != 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.
You are no longer using this preference. But (unlike with the case of macros, a more rarely used expert feature) we do not want to ignore the problem of compatibility of old user preferences, in case someone already preferred grayscale.
So where you do use an EnumSetting
, you should also supply this old path as the optional constructor argument.
You should also then override the Migrate() method, because the default implementation won't do the right thing. The old possible values of 0 and 1 will not map identically to the new enumerators. Rather 0 should go to Color (new), which we want to make the default colored spectrogram, but 1 should go to Inverse Grayscale, which is the "old" grayscale, in case someone did save that preference.
So see EnumSettingBase::Migrate()
and adapt it. Then test this by choosing grayscale as built in head, then run your build.
See audacity.cfg for where the preferences are stored. Observe the relevant lines inside it.
https://manual.audacityteam.org/man/preferences.html#stored
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.
Done.
const EnumValueSymbols &SpectrogramSettings::GetColorSchemeNames() | ||
{ | ||
static const EnumValueSymbols result{ | ||
// Keep in correspondence with enum SpectrogramSettings::ColorScheme: |
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.
Audacity's designers are deciding which exact English strings should be shown to the user, and internationalized.
Meanwhile, you should also rewrite this table using the two-argument constructor for EnumValueSymbol
. For instance
{ L"Default", XO("Color (New)") }
The first string, which is not internationalized, can be stable across Audacity versions, identifying the choice in saved preferences, while the user-visible name might be changed.
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.
Also, make a non-static assertion about that correspondence, like
wxASSERT(result.size() == csNumColorScheme);
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.
Done
@@ -101,7 +101,7 @@ enum { | |||
ID_GAIN, | |||
ID_RANGE, | |||
ID_FREQUENCY_GAIN, | |||
ID_GRAYSCALE, | |||
ID_COLOR_SCHEME, |
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.
No problems in this file
@@ -215,7 +215,7 @@ void DrawClipSpectrum(TrackPanelDrawingContext &context, | |||
freqHi = selectedRegion.f1(); | |||
#endif | |||
|
|||
const bool &isGrayscale = settings.isGrayscale; | |||
const int &colorScheme = settings.colorScheme; |
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.
no problems in this file
src/prefs/SpectrogramSettings.h
Outdated
bool isGrayscale; | ||
typedef int ColorScheme; | ||
enum ColorSchemeValues : int { | ||
// Keep in correspondence with AColor::gradient_pre |
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.
You mean AColor::colorSchemes
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.
Both of them.
As for that, I share those hopes. I not only hope so, I expect so, from what I know. Though that is on the understanding that code of the Audacity audio editor "remaining free" is consistent with the possibility of a dual sub-licensing of parts of it that the CLA leaves open. |
* Update UnusedStrings.h Added translatable strings for crash/error reporting and the new spectrogram colour preferences (#830) * Update UnusedStrings.h updated as per guidance from Paul * Update UnusedStrings.h Commented out Paul's examples, updated the spectogram settings as per Steve's/Peter's suggestion - Color (Default) and Color (Classic) * New crash reporter strings; access keys; context strings * Added "Unknown assertion" and more context strings * Added comments about shortcut keys * Choice control items do NOT need & characters, choice label does Co-authored-by: Paul Licameli <paul.licameli@audacityteam.org>
@Paul-Licameli Ping |
I have not forgotten this! I will write more today. |
* Update UnusedStrings.h Added translatable strings for crash/error reporting and the new spectrogram colour preferences (audacity#830) * Update UnusedStrings.h updated as per guidance from Paul * Update UnusedStrings.h Commented out Paul's examples, updated the spectogram settings as per Steve's/Peter's suggestion - Color (Default) and Color (Classic) * New crash reporter strings; access keys; context strings * Added "Unknown assertion" and more context strings * Added comments about shortcut keys * Choice control items do NOT need & characters, choice label does Co-authored-by: Paul Licameli <paul.licameli@audacityteam.org>
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.
Changes to user-visible strings because of string freeze
}; | ||
|
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.
Thank you for these changes.
But now, we are in the period of "string freeze." That means the message catalog locale/audacity.pot, in which all the XO (and XC) strings are extracted, is in a final form, allowing translators to finish their work. One of our designers reviewed this project, and specified changes to the user-visible strings.
So you must:
- change the XO strings above to the XC strings below. (But leave the wxT strings as they are.)
- Find these strings in lib-strings/locale/UnusedStrings.h; be sure you have copied them exactly; and delete them from that file.
- Rebase this branch onto more recent master. (And while doing that, do an interactive rebase, and make sure all commits in the sequence compile. I believe the first one does not. And maybe do some fixups to shorten the list of commits.)
- Test (using one of the completed languages, such as Dutch) that translation works. (You change language using the Interface page of Preferences.)
- Force-push your rebased branch.
XC("Color (default)", "spectrum prefs"),
XC("Color (classic)", "spectrum prefs"),
XC("Grayscale", "spectrum prefs"),
XC("Inverse grayscale", "spectrum prefs"),
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.
Done
src/prefs/SpectrumPrefs.cpp
Outdated
@@ -223,11 +223,12 @@ void SpectrumPrefs::PopulateOrExchange(ShuttleGui & S) | |||
S.Id(ID_FREQUENCY_GAIN).TieNumericTextBox(XXO("High &boost (dB/dec):"), | |||
mTempSettings.frequencyGain, | |||
8); | |||
|
|||
S.Id(ID_COLOR_SCHEME).TieChoice(XXO("Color Sche&me:"), |
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.
Also for string freeze, the string above must be exactly as the designer specified in UnusedStrings.h. Cut and paste this string from there (with the comment too, pasted to the line before the line with the string):
// i18n-hint Scheme refers to a color scheme for spectrogram colors
XC("Sche&me", "spectrum prefs"),
src/commands/SetTrackInfoCommand.cpp
Outdated
{ | ||
S.SetStretchyCol( 2 ); | ||
auto schemes = SpectrogramSettings::GetColorSchemeNames(); | ||
S.Optional( bHasSpecColorScheme).TieChoice( XXO("Spectro. Color scheme:"), mSpecColorScheme, |
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.
Also for string freeze, it's too late now to add new user-visible strings -- or else they won't translate from English. So change this to reuse the same string as in the preferences dialog:
XC("Sche&me", "spectrum prefs")
- Add color scheme preference Add 'Spectro. Color scheme' to 'Set Track Visuals' macro command
- Add some assertion - Define spectrogram color scheme name using the two-argument constructor. (i18n)
… new 'Color Scheme' choice
I tested with pure Spectrogram and with Multi-view dor the new and alternative colorways |
Thanks, Peter. Did you also observe the changes in Set Track Visuals? A choice replaces what was only a checkbox for grayscale. |
Paul, I assume you mean from the TCp - in which case yes, I have been playing with that too and all looks fine. |
No, I refer to one of the Macro commands. See #969 |
I'm pleased with these changes to address my points. Each commit builds. I am satisfied too now with changes in AColor.cpp. (I might suggest that the names in AColorResources.h could be made static, to have no linkage and spare a little space in symbol tables.) But I do not want the change in Experimental.h. It leaves no visual indication of sync lock in a spectrogram. If you think this change is very important, than I think some alternative solution should be sought. The sync lock tiles might be painted more than once (as they are in Waveform view). They might be repainted last, with transparency. Perhaps you fill follow up this implementation suggestion. So to summarize: please rebase onto most master, and omit the change to Experimental.cmake. Then I will approve it and merge it! |
Removing transparency from spectrogram is important fix! Without the change, new colormap gonna lose its power. |
I logged this as P3 Bug #2806 And remember that this is not a result of the new Spectrogram colorway - it's always been like this. |
As your branch is now, there is no indication at all of sync lock, rather than the existing one, poor though that might be. Why do you think removing transparency of the spectrogram is very important? In my own trials building your branch, I did not think it looked bad either with or without it. Would you consider merging the branch with transparency, but opening an issue to correct the problems? I see your example in "Obstacles." https://forum.audacityteam.org/viewtopic.php?f=20&t=117875#p424607 |
I don't think that losing the Sync-Lock "clocks" in spectrogram view is a problem. Even with "classic" spectrogram view they are barely visible with real world audio, and imo they are little more than visual clutter when they are. Waveform view, or multi-view are far better suited to temporal editing. I'd rather have a clearer view of the spectrogram in track spectrogram view than overlaid information that is irrelevant to "spectral" editing. |
Transparency makes the visual totally different from what I intended(accurate and pretty). Bringing clock overlay on top of spectrogram like this might be the fix. |
If we want to keep the clocks but also make spectrogram non-transparent, then I think a solution is to draw the clocks with transparency, not the track, and paint the clocks later. If we lose all of the clocks in spectral view, arguably that's a regression. But maybe Steve and Peter would agree not to treat it as such. Are there examples of clocks on the spectrogram in the manual? If so they would be one more thing to change in manual images. I think the objection that clocks are clutter for spectral editing is irrelevant. The track that you do interact with, for spectral editing, never shows the clocks. The clocks indicate what part of another track will be affected, if the track you do interact with changes its length with insertion or deletion. Spectral editing doesn't do that, but other edits do. |
How did you do that "like this" picture? Did you change Audacity source code to do it? |
No. I used image editor. |
It's not really our call any more - But I would certainly be happy with the way it si now with the closcks invisible on the spectrogram (I agree with Steve's views above that So if MUSE QA and RM agree (Dmitry, Martin, Jouni) I would vote for this - and this would then let me close P3 Bug 2806. As far as I can recall no user has ever complained about the clocks on Spectrograms - porr visibiliy of or lack thereof. |
I would leave bug 2806 open. But we need to get a release out very soon, and these new colors will be a banner feature, and I don't want this detail to block it. Maybe Myungchul will even write the fix easily. I outlined how we might to do it. Repaint the clocks, after the spectrum, making the clocks the transparency. |
Thanks for trying the clocks dofuuz, but from a user perspective I would much prefer to NOT have the clocks. When using track spectrogram view and Sync-Lock, I would have to temporarily turn off Sync-Lock to remove the distraction of the clocks. That's more work for the user, and a needless distraction from their workflow. The user can already see if Sync-Lock is on by looking for the clock in the control panel on the left hand end of the track - which is how users have had to work (with zero complaints) when using the "classic" spectrogram color scheme. With QA hat on, usability trumps consistency. Now that I can see what it looks like with the new color scheme + clocks (thank you), I would say that it is significantly worse than not having the clocks showing in spectrograms. Steve |
Thank you, Steve. You have a strong opinion about the clocks. I do NOT have a strong opinion about them -- which is why I brought the matter up: in the absence of strong opinions, the presumption should be conservatism about the application's behavior, changing only what one needs to change for the really new feature. In this case, hiding the clocks was not the intention of the feature, but a side-effect of trying to achieve something else, the non-transparency that improved spectrogram appearance. However, given your remarks now, I find no reason left to delay the merger of this pull request. If someone really wants to overrule the elimination of the clocks, let them raise an issue. |
Merged! The residual issue about the clocks -- if anyone insists on keeping them -- is already at: https://bugzilla.audacityteam.org/show_bug.cgi?id=2806 |
Great😆 |
dofuuz, do you wish to be listed in the Credits dialog of the next version? If so, how exactly should we spell your name? (In Hangul too if you like.) |
@Paul-Licameli |
I have updated the Website Credits page adding Keum to the "Contributors" I cannot update the in-app credits. Thanks for your work on this Keum/dofuuz 😆 |
Please help, we want to be sure that it is correct to alphabetize that under K and not M! |
Keum is my family name. |
Thanks for that clarification dofuuz ( Myungchul) - I have adjusted that ordering in the Website credits: Th latest 3/.0.3 alpha already has the correct alphabetic ordering. Peter. |
* Update UnusedStrings.h Added translatable strings for crash/error reporting and the new spectrogram colour preferences (audacity#830) * Update UnusedStrings.h updated as per guidance from Paul * Update UnusedStrings.h Commented out Paul's examples, updated the spectogram settings as per Steve's/Peter's suggestion - Color (Default) and Color (Classic) * New crash reporter strings; access keys; context strings * Added "Unknown assertion" and more context strings * Added comments about shortcut keys * Choice control items do NOT need & characters, choice label does Co-authored-by: Paul Licameli <paul.licameli@audacityteam.org>
I made a post for this PR:
https://forum.audacityteam.org/viewtopic.php?f=20&t=117875
Same content, but with code for generating the colormap:
https://github.com/dofuuz/audacity-colormap