Skip to content

make sure effect chains are initialized properly#1241

Merged
daschuer merged 1 commit intomixxxdj:masterfrom
Be-ing:effect_loading_fix
Apr 30, 2017
Merged

make sure effect chains are initialized properly#1241
daschuer merged 1 commit intomixxxdj:masterfrom
Be-ing:effect_loading_fix

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Apr 19, 2017

if effects.xml is a valid XML file but does not contain enough elements like this. That should never happen, but in case it does, this change will catch it.

if effects.xml is a valid XML file but does not contain enough
<EffectChain> elements
@daschuer
Copy link
Copy Markdown
Member

Thank you for this quick band aid.

It is still possible to create too many effects.

Did you consider decoupling slot creating and effect loading?
It looks wrong, that a file for storing effects is also responsible for setting up the slots including their Control Objects. Will it be a big change?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Apr 19, 2017

Putting more than 4 <EffectChain> elements in the XML file does not seem to have any negative impact.

Decoupling EffectChainSlot creation and loading state from XML would be a bit complicated. This works as is.

@daschuer
Copy link
Copy Markdown
Member

daschuer commented Apr 20, 2017

Putting more than 4 elements in the XML file does not seem to have any negative impact.

I have not tested it yet, but I assume that there are additional effects are added to new effect slots created along with there Control Objects. We should avoid it to not give the impression that the number of effects slots can be set by the XML file.

If we need a dynamic number of effect slots, we should allow the skins to set them up not the effect.xml which is IMHO only responsible for the content of the effect slots.

@daschuer
Copy link
Copy Markdown
Member

Decoupling EffectChainSlot creation and loading state from XML would be a bit complicated. This works as is.

It is OK to me to keep the current situation. To clarify, do you agree that it should be decoupled finally.
If yes, we should issue a bug for it.

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Apr 28, 2017

It is OK to me to keep the current situation. To clarify, do you agree that it should be decoupled finally.
If yes, we should issue a bug for it.

I don't think it matters much either way. If you want to open a bug for it, go ahead.

@daschuer
Copy link
Copy Markdown
Member

Thank you! LGTM

The bug is here:
https://bugs.launchpad.net/mixxx/+bug/1687320

@daschuer daschuer merged commit 51d9343 into mixxxdj:master Apr 30, 2017
@Be-ing Be-ing deleted the effect_loading_fix branch May 11, 2017 07:52
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.

2 participants