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

Change certain strings based on context #1848

Closed
mkruselj opened this issue May 9, 2020 · 8 comments · Fixed by #1924
Closed

Change certain strings based on context #1848

mkruselj opened this issue May 9, 2020 · 8 comments · Fixed by #1924
Labels
UX Issues related to user experience (UX) - mouse, touch, keyboard, MIDI inputs, etc.
Milestone

Comments

@mkruselj
Copy link
Collaborator

mkruselj commented May 9, 2020

For example, LFO Phase/Shuffle parameter - it should be just called Phase for all LFO modes except Step Seq, otherwise it should be called Shuffle.

Another thing, currently pitch-based parameters (Pitch, Sync, Formant) don't show any unit at all when it should be semitones, but this should change between "semitones" if microtuning is not loaded and "keys" or "pitches" (I'm more inclined towards the former) if microtuning is loaded.

In fact, it looks like those two things are the only of this sort which would need a context-sensitive update.

Also tightly related to #1201!

@mkruselj mkruselj added the UX Issues related to user experience (UX) - mouse, touch, keyboard, MIDI inputs, etc. label May 9, 2020
@baconpaul
Copy link
Collaborator

The primary problem @mkruselj ran into trying this first was that Parameter doesn’t have a reference to its associated storage (but does vice versa). Adding this in carefully and making sure it is set will let us do this. Or maybe doing some gross hack. But much bigger change than just an if alas.

@mkruselj
Copy link
Collaborator Author

mkruselj commented May 10, 2020

Another thing to tuck into this issue is changing "semitones" unit in pitch-based parameters to "keys", based on microtuning being loaded or not.

EDIT: Actually, this is issue #1495, so let's link these two together!

@baconpaul baconpaul added this to the 1.7 beta 1 milestone May 13, 2020
@renatoviana12
Copy link

Hey, can I pick this up?

@baconpaul
Copy link
Collaborator

you mean pick up the issue to code? Or pick up the binary with the change?

This issue is a bit hairy as a first issue but happy to describe how to do it if you are a C++ dev looking to join the project!

@renatoviana12
Copy link

Pick up the issue, I would love to join the project :)

@baconpaul
Copy link
Collaborator

baconpaul commented May 13, 2020

Cool! Welcome. You probably should also join our slack then if you want to hang with the team.

The basic problem is the data structure which knows if we are tuned (SurgeStorage) is "above" the data structure which makes the string (Parameter) in the hierarchy. So we need to get a reference to the storage object into the parameter function which makes the string.

There's a lot of ways to do it but I think the safest is this:

  1. Make sure you can build and run the synth. A good test is to edit one of the strings in CAboutBox.cpp to be something funny, build, run in your DAW, do help/about, and see if you see it. Also check out doc/git-howto.md on how we use git. If you have this running so you can make and see changes in a cycle then:

  2. src/common/Parameter.h add a forward decl of SurgeStorage. You can't include surge storage.h there it will cycle.

  3. Parameter.h add a SurgeStorage *s = nullptr argument to get_display at the end of the arg list. You may also want to add a version with signature char *, SurgeStorage* which just calls getParameter( c, false, 0, s )

  4. src/common/Parameter.cpp include SurgeStorage.h and where there is semitones do an if( s && ! s->isStandardTuning ) "keys" : "semitones" at the right place

  5. src/common/gui/SurgeGUIEditor.cpp find the places where we call get_display and add add the reference (and the defaults for external and ef I suppose also). In SGE the reference will look like &( synth->storage )

  6. test, push, etc...

And welcome! Glad to have devs join.

@renatoviana12
Copy link

I'll get the proper tooling and get to it. I'm also joining the team slack rn

@mkruselj mkruselj added the Code Hints Requested Someone would like to tackle this but wants a few pointers on how to start label May 14, 2020
@baconpaul baconpaul removed the Code Hints Requested Someone would like to tackle this but wants a few pointers on how to start label May 19, 2020
@baconpaul
Copy link
Collaborator

I went a different approach on this since it showed up more than once. Pushing the pattern today.

baconpaul added a commit to baconpaul/surge that referenced this issue May 22, 2020
We want to make formating decisiosn based on the state of the synth
so parameters need storages. They don't always have them so add
a weak reference to storage to the parameter.

Demosntrate this by changing 'semitones' labels to 'keys' when
you have an alternate tuning. Closes surge-synthesizer#1848
baconpaul added a commit that referenced this issue May 22, 2020
We want to make formating decisiosn based on the state of the synth
so parameters need storages. They don't always have them so add
a weak reference to storage to the parameter.

Demosntrate this by changing 'semitones' labels to 'keys' when
you have an alternate tuning. Closes #1848
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UX Issues related to user experience (UX) - mouse, touch, keyboard, MIDI inputs, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants