Conversation
Kodiai Review SummaryWhat ChangedUpdates ProjectM from v3 to v4 with new C API, updates dependencies (GLM, angle, dlfcn-win32, dirent), and migrates addon to use projectM 4.0 API. Reviewed: core logic, docs Strengths
ObservationsImpact[CRITICAL] src/Main.cpp (456): Missing break statement causes case 5 to fall through to case 6 [MAJOR] src/Main.cpp (263): Logic error always returns same value Verdict🔴 Address before merging -- 2 blocking issue(s) found Review Details
|
7eec273 to
783123c
Compare
5309f49 to
b83c586
Compare
|
@garbear I played a bit with this and at least on android I was not able to activate other presets. Despite of what I selected the default preset - projectM - was used, even after restarting Kodi. Otherwise the add-on seems to work just fine. |
depends/windows/dirent/dirent.sha256
Outdated
| @@ -1 +1 @@ | |||
| 4bcf07266f336bcd540fec5f75e90f027bd5081d3752f9ea5d408ef6ae30a897 | |||
| 6ffcc318f00be192acb611c58aa58ee0cfd96776010680d7f38cf24f3dd8baf9 | |||
There was a problem hiding this comment.
projectM 4 no longer uses dirent on Windows, instead we've switched to std::filesystem, which is more convenient. So this dependency can probably be removed.from the plug-in.
| void CVisualizationProjectM::PresetSwitchedEvent(bool isHardCut, unsigned int index, void* context) | ||
| { | ||
| auto that = reinterpret_cast<CVisualizationProjectM*>(context); | ||
| that->m_lastPresetIdx = static_cast<int>(projectm_playlist_get_position(that->m_playlist)); |
There was a problem hiding this comment.
The plug-in should probably save the last preset index in the settings as well here, so playback will continue at the same index on the next load.
|
The Windows build issues should all be sorted with the 4.2 release as we removed SOIL2 and also switched from GLEW to glad for OpenGL function resolving, including a new API that allows the outside app to provide the GL function pointers if needed. We verified it works with Angle, so it should make things easier for Kodi as well! |
98e0f7b to
7a8c084
Compare
When is 4.2 due? Currently I'm based on master on top of 4.1.6 but it might have these changes?
So I should drop soil and add glad as a dependency?
I also dropped dirent from the dependencies.
I haven't touched the C++ work, it comes directly from #130. If any changes are suggested, I'd appreciate if they could be done by someone. My contribution is basically the dependencies, so maybe an actual C++ author can open a new PR with my dependency work. |
When I'm back home in a week, I can have a look at it and add any necessary changes to the other PR (I've opened it, so that's easy). Or just pull in your changes to the other PR or rebase on this PR after it's been merged, whatever is easier! |
Description
This PR updates all dependencies, including a bump of projectM from v3 to v4.
All non-dependency changes are currently from #130. Special thanks to @kblaschke for the v4 work.
Motivation and context
We currently have no ProjectM add-on for anything but Windows: https://jenkins.kodi.tv/blue/organizations/jenkins/xbmc%2Fvisualization.projectm/detail/Piers/49/pipeline/162
Windows doesn't build here yet, but we can release this without Windows so that ProjectM won't be missing on any platforms anymore.
How has this been tested?
Builds and runs on my macbook M2:
Windows testing in progress.