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) Controller: clear mapping pointer when unloading a mapping #13907

Open
wants to merge 1 commit into
base: 2.5
Choose a base branch
from

Conversation

ronso0
Copy link
Member

@ronso0 ronso0 commented Nov 18, 2024

With #13842 I noticed that the controller's pMapping still points to the previously loaded mapping when loading 'No mapping'.
Is there any reason for that?

@ronso0 ronso0 changed the title Controller: clear mapping pointer when unloading a mapping (fix) Controller: clear mapping pointer when unloading a mapping Nov 18, 2024
Comment on lines 45 to 48
void setMapping(std::shared_ptr<LegacyControllerMapping> pMapping) override;
void clearMapping() override {
m_pMapping = nullptr;
}
Copy link
Member

Choose a reason for hiding this comment

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

just call setMapping with a nullptr directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

That was the first thing I tried.
Gives me this when selecting No mapping

DEBUG ASSERT: "pMapping.use_count() == 1" in function std::shared_ptr<_Tp> Controller::downcastAndTakeOwnership(std::shared_ptr<LegacyControllerMapping>&&) [with SpecificMappingType = LegacyMidiControllerMapping] at ./src/controllers/controller.h:89

Copy link
Member Author

Choose a reason for hiding this comment

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

std::shared_ptr<SpecificMappingType> downcastAndTakeOwnership(
std::shared_ptr<LegacyControllerMapping>&& pMapping) {
// Controller cannot take ownership if pMapping is referenced elsewhere because
// the controller polling thread needs exclusive accesses to the non-thread safe
// LegacyControllerMapping.
// Trying to cast a std::shared_ptr to a std::unique_ptr is not worth the trouble.
VERIFY_OR_DEBUG_ASSERT(pMapping.use_count() == 1) {
return nullptr;
}
auto pDowncastedMapping = std::dynamic_pointer_cast<SpecificMappingType>(pMapping);
VERIFY_OR_DEBUG_ASSERT(pDowncastedMapping) {
return nullptr;
}
return pDowncastedMapping;
}

Copy link
Member

Choose a reason for hiding this comment

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

right, that function does not take that usecase into account. Simply add a if (pMapping == nullptr) { return nullptr; } so it supports that usecase. Alternatively you can add the appropriate handling to the setMapping implementations.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. I only took a brief look at this and didn't want to touch it.
But yeah, that's a straight forward solution.
Thanks!

@Swiftb0y
Copy link
Member

Swiftb0y commented Nov 18, 2024

Is there any reason for that?

Not that I know of, possibly lost during refactoring?

@ronso0 ronso0 force-pushed the controller-clear-mapping-pointer branch from eb6f94a to e6fb932 Compare November 19, 2024 14:52
Copy link
Member

@Swiftb0y Swiftb0y left a comment

Choose a reason for hiding this comment

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

conceptually LGTM. Thank you. Waiting for CI.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants