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

Fix #22382 Part 2: Add new preference to play notes on muted tracks when editing #25683

Conversation

pacebes
Copy link
Contributor

@pacebes pacebes commented Nov 29, 2024

Resolves: #22382 Part 2

This pull request implement the second (and last) part of the functionality as described in "2. Add a new preference to control whether, when the above preference is ON, you hear notes added/clicked/selected on instruments that are muted in the mixer." section

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@pacebes pacebes force-pushed the 22382-addNewPreferencePlayNotesOnMutedMixerTracksWhenEditing branch from 0157868 to e5748bc Compare November 29, 2024 14:39
@cbjeukendrup
Copy link
Contributor

About the macOS build error: it looks like the problem is that somehow draw/types/geometry.h gets transitively included in audiomodule.cpp, which also transitively includes MacTypes.h (via osxaudiodriver.h), which contains types that conflict with the types from geometry.h. The solution would be to make sure that audiomodule.cpp can't transitively see draw/types/geometry.h. Tip: in Qt Creator, in the side panel, you can see the include hierarchy of a file (https://doc.qt.io/qtcreator/creator-include-hierarchy-view.html), which is going to be very helpful with this problem.

Slight complication: because of unity builds, it is also possible that geometry.h is transitively seen by audiomodule.cpp itself, but by another cpp file from the audio module that lands in the same unit as audiomodule.cpp. The unit files can be found named as Unity_01.cxx in the relevant subfolder of the build directory.

@pacebes pacebes force-pushed the 22382-addNewPreferencePlayNotesOnMutedMixerTracksWhenEditing branch 2 times, most recently from f58a5b8 to fd0a14c Compare November 30, 2024 14:36
@pacebes
Copy link
Contributor Author

pacebes commented Nov 30, 2024

About the macOS build error: it looks like the problem is that somehow draw/types/geometry.h gets transitively included in audiomodule.cpp, which also transitively includes MacTypes.h (via osxaudiodriver.h), which contains types that conflict with the types from geometry.h. The solution would be to make sure that audiomodule.cpp can't transitively see draw/types/geometry.h. Tip: in Qt Creator, in the side panel, you can see the include hierarchy of a file (https://doc.qt.io/qtcreator/creator-include-hierarchy-view.html), which is going to be very helpful with this problem.

Slight complication: because of unity builds, it is also possible that geometry.h is transitively seen by audiomodule.cpp itself, but by another cpp file from the audio module that lands in the same unit as audiomodule.cpp. The unit files can be found named as Unity_01.cxx in the relevant subfolder of the build directory.

Qt Creator didn't seem to work for me. I chose a different path with cygwin and g++:

gcc -DNO_QT_SUPPORT -H -fsyntax-only -I . -I ./src/ -I ./src/framework/global -I ./src/framework -I ./src/framework/audio -I /cygdrive/c/Qt/6.2.4/msvc2019_64/include/QTCore -I /cygdrive/c/Qt/6.2.4/msvc2019_64/include -I /cygdrive/c/Qt/6.2.4/msvc2019_64/include/QtGui/ ./src/framework/audio/audiomodule.cpp >IncludesHierarchy 2>&1

The problem has been caused by the inclusion of "iplaybackcontroller.h" within mixerchannel.h

. ./src/framework/audio/internal/audiooutputdevicecontroller.h
.. ./src/framework/audio/internal/worker/iaudioengine.h
... ./src/framework/audio/internal/worker/mixer.h
.... ./src/framework/audio/internal/worker/mixerchannel.h
..... ./src/playback/iplaybackcontroller.h
...... ./src/notation/inotation.h
....... ./src/notation/internal/inotationundostack.h
........ ./src/notation/notationtypes.h
......... ./src/engraving/dom/articulation.h
.......... ./src/engraving/dom/chordrest.h
........... ./src/engraving/dom/../types/types.h
............ ./src/framework/draw/types/geometry.h


.......... ./src/engraving/dom/chordrest.h
........... ./src/engraving/dom/../types/types.h
............ ./src/framework/draw/types/painterpath.h
............. ./src/framework/draw/types/geometry.h


.......... ./src/engraving/dom/chordrest.h
........... ./src/engraving/dom/durationelement.h
............ ./src/engraving/dom/engravingitem.h
............. ./src/framework/draw/painter.h
.............. ./src/framework/draw/types/geometry.h

I have included "iplaybackcontroller.h" within mixerchannel.h to use PlaybackController::isPlaying() method to determine if mixer options should be ignored.

(Edited)
Some options come to mind, but I'm not sure if they are "acceptable":

  • Option 1.- Add to audiomodule.cpp something to avoid the inclusion of geometry.h:
#ifdef Q_OS_MACOS
#define MUSE_DRAW_GEOMETRY_H
#endif

This may cause some future problems if you change "#ifndef MUSE_DRAW_GEOMETRY_H" clauses to "#pragma once"at the beginning of the include files

  • Option 2.- Change playbackcontroller.cpp to emit a notification to mixer whenever isPlaying() changes. It duplicates somehow isPlaying() function, but I don't see any other way to solve it.

  • Option 3.- Change geometry.h to avoid only the definition that cause problems (for the time being)

#ifndef Q_OS_MACOS
using Point = PointX<int>;
#endif
....

#ifndef Q_OS_MACOS
class Rect : public RectX<int>
{
public:
    inline Rect()
        :  RectX<int>() {}
#ifndef NO_QT_SUPPORT
    inline Rect(const QRect& r)
        : RectX<int>(r.x(), r.y(), r.width(), r.height()) {}
#endif
    inline Rect(int x, int y, int w, int h)
        : RectX<int>(x, y, w, h) {}

    inline RectF toRectF() const { return RectF(x(), y(), width(), height()); }
};
#endif

  • Option 4: follow some suggestions and use namespaces as suggested in (P.S. https://community.khronos.org/t/questions/48794). ... to avoid conflict with mac definitions, but I don't know if this is something that could be done or if it makes sense.

Would you please recommend me which one can I use of if you have a better proposal ?

Thanks.

@pacebes
Copy link
Contributor Author

pacebes commented Dec 1, 2024

@pacebes pacebes force-pushed the 22382-addNewPreferencePlayNotesOnMutedMixerTracksWhenEditing branch from fd0a14c to f86b12a Compare December 1, 2024 07:41
@pacebes
Copy link
Contributor Author

pacebes commented Dec 1, 2024

I finally figured out where the problem was. "using namespaces" clauses should be after includes . Therefore, I changed the following lines after all the includes section within audiomodule.cpp:

using namespace muse;
using namespace muse::modularity;
using namespace muse::audio;
using namespace muse::audio::synth;
using namespace muse::audio::fx;

I've been looking at it looks like it might be good practice to #include lines before namespace definitions. You might get unexpected results by doing the opposite
.

@cbjeukendrup
Copy link
Contributor

Oh, yes, includes should certainly almost always come before anything else. Good find!

bool MixerChannel::playNotesOnMutedTracksWhenEditing() const
{
// We are not muted if we are Editing and we want to play notes even if mixer sound is off
return !playbackController()->isPlaying()
Copy link
Contributor

Choose a reason for hiding this comment

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

Unfortunately, this is incorrect for many reasons:

  1. this creates bad (circular) dependencies between the audio and playback modules. The audio module must not know anything about the playback module, as the latter is a higher-level one
  2. this code is thread-unsafe
  3. constantly accessing these settings may cause performance degradation

@RomanPudashkin
Copy link
Contributor

I'm going to close this PR as the feature itself is rather niche. And we can't sacrifice audio performance/stability because of it. Also, it should be taken by the in-house team as the entry-level for the audio engine is very high

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.

Shortcut for Preferences>Note Input>Play Notes When Editing
3 participants