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.

Sign up for GitHub

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

Jump to bottom

Use resid submodule from libsidplayfp #6883

Merged
merged 14 commits into from
Sep 23, 2023
Merged

Use resid submodule from libsidplayfp #6883

merged 14 commits into from
Sep 23, 2023

Conversation

sakertooth
Copy link
Contributor

Switch the resid submodule to use the one from libsidplayfp. This seems to be a bit more updated, and puts its declarations in a namespace, which fixes a problem involving conflicting declarations with other header files.

DomClark
DomClark reviewed Sep 23, 2023
View reviewed changes
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

It looks like the CMake files are indented with spaces, but we prefer tabs for these, like our C++ code.

plugins/Sid/SidInstrument.cpp Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/resid/CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/CMakeLists.txt Outdated Show resolved Hide resolved
DomClark
DomClark reviewed Sep 23, 2023
View reviewed changes
Copy link
Member

@DomClark DomClark left a comment

Choose a reason for hiding this comment

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

This is basically there, I think. Presumably you've tested some SID patches and it sounds the same as before?

CMakeLists.txt Outdated Show resolved Hide resolved
plugins/Sid/CMakeLists.txt Outdated Show resolved Hide resolved
@sakertooth
Copy link
Contributor Author

This is basically there, I think. Presumably you've tested some SID patches and it sounds the same as before?

Thanks for reminding me. Using filter8580new.cc instead of filter.cc broke the instrument. It should be good now, though I will do more testing with specific patches and notify you here when I'm done.

@sakertooth
Copy link
Contributor Author

Okay, I tested all the SID/*.xpf patches that come with LMMS, and all of them sound fine.

@sakertooth sakertooth merged commit 49c713d into LMMS:master Sep 23, 2023
9 checks passed
@sakertooth sakertooth deleted the change-resid-submodule branch September 23, 2023 23:33
consolegrl pushed a commit to consolegrl/lmms that referenced this pull request Sep 27, 2023
@sakertooth @consolegrl
Use resid submodule from libsidplayfp (LMMS#6883)
1a90f3d
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DomClark DomClark DomClark approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@sakertooth @DomClark