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

Effects manifest defaults #10837

Merged
merged 3 commits into from
Mar 29, 2023
Merged

Conversation

Swiftb0y
Copy link
Member

fixes #10834

@Swiftb0y Swiftb0y added this to the 2.4.0 milestone Aug 29, 2022
Comment on lines 117 to 118
// overwrite our parameter by taking a copy of the same parameter from `other`
*currentParameterIt = parameterToCopy;
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually I just realized, does this create a dangling reference? How do I force a copy here so m_effectParameterPresets takes ownership?
Does this suffice?

Suggested change
// overwrite our parameter by taking a copy of the same parameter from `other`
*currentParameterIt = parameterToCopy;
// overwrite our parameter by taking a copy of the same parameter from `other`
*currentParameterIt = EffectParameterPreset(parameterToCopy);

Copy link
Member

Choose a reason for hiding this comment

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

I think the proposed version is redundant because the copy constructor is already used.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thinking more about it, I'm pretty sure its the copy-assignment operator, so we should be good then.

@Swiftb0y Swiftb0y requested a review from Be-ing August 29, 2022 20:45
@Be-ing
Copy link
Contributor

Be-ing commented Aug 29, 2022

This seems like a reasonable solution.

@daschuer daschuer changed the base branch from main to 2.4 January 12, 2023 07:02
@Swiftb0y
Copy link
Member Author

Swiftb0y commented Mar 8, 2023

Friendly ping. Also, are we concerned about the $O(n^2)$ complexity? 3rd-party effect plugins could easily have hundreds of parameters.

@daschuer
Copy link
Member

daschuer commented Mar 8, 2023

Oh thank you for the reminder. This has slipped through.

DEBUG_ASSERT(backendType() == other.backendType());

// technically algorithmically inefficient solution O(n²). May be
// optimizable by sorting first, gains depend on parameter count
Copy link
Member

@daschuer daschuer Mar 8, 2023

Choose a reason for hiding this comment

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

I think the best way would be to make m_effectParameterPresets a QHash.
Than the std::find_if becomes a fast:
auto currentParameterIt = m_effectParameterPresets.find(parameterToCopy.id());

Copy link
Member Author

Choose a reason for hiding this comment

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

Can you clarify? Do you mean that we change m_effectParameterPresets to be a QHash instead of a QList? That won't work because we need to preserve the ordering of m_effectParameterPresets. What would work though is if we just built a temporary QHash on each call. What do you think?

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Looks already good. Do you have interest to interest to implement the suggestion?
Or should I merge this now? Than an updated comment would be nice.

@Swiftb0y
Copy link
Member Author

Swiftb0y commented Mar 8, 2023

I'd like to address the complexity issue first.

@Swiftb0y Swiftb0y marked this pull request as draft March 8, 2023 23:28
@daschuer
Copy link
Member

daschuer commented Mar 9, 2023

Oh, yes. I have just realized that this does not work, because a stored custom sort order is lost.
Maybe we need a different approach.
A temp QHash may help to identify lost parameters.

@Swiftb0y
Copy link
Member Author

Tested and ready for review. I think the new code is even simpler.

@Swiftb0y Swiftb0y marked this pull request as ready for review March 13, 2023 17:16
@Swiftb0y
Copy link
Member Author

@daschuer merge? I tried experimenting with parallel algorithms but we need to link intel tbb for that and I don't feel like bothering with that right now.

Copy link
Member

@daschuer daschuer left a comment

Choose a reason for hiding this comment

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

Nice Solution. LGTM thank you.

@daschuer daschuer merged commit ce264ae into mixxxdj:2.4 Mar 29, 2023
@Swiftb0y Swiftb0y deleted the effects-manifest-defaults branch March 29, 2023 20:31
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.

Effect Parameters are not shown when their Manifest has been changed
3 participants