feat: improve screen rendering framework #13737
Conversation
(cherry picked from commit ad3bf1a)
github-actions
bot
added
controller mappings
build
code quality
qml
controller backend
labels
| } else if (oneShot) { | ||
| } else if (oneShot && CmdlineArgs::Instance().getControllerDebug()) { | ||
| qCDebug(m_logger) << "Starting one-shot timer:" << timerId; | ||
| } else { | ||
| qCDebug(m_logger) << "Starting timer:" << timerId; |
There was a problem hiding this comment.
this seems half backed now, either check it for both or none. Though shouldn't it be unnecessary to check, because the logging category takes care of it?
There was a problem hiding this comment.
Ah indeed, needs more doing.
By category, I assume you mean the debug category? If yes, this is unfortunately not enough about the noisy logging related to controller communication is blindly fed to the debug category everywhere on the code base.
There was a problem hiding this comment.
ah, yeah, thats unfortunate. I think we have some code somewhere though that adjusts the default logging category. That should get rid of the controller debug code in theory. Maybe thats broken? Can you look into it?
There was a problem hiding this comment.
Fair enough, I didn't realise this was supposed to be a feature since #4523
I'm not sure if it doesn't work as expected or if I misunderstood the point of having a dedicated CLI arg - are we expecting controller debug message to show when running with --log-level debug?
There was a problem hiding this comment.
I'm not sure if it doesn't work as expected or if I misunderstood the point of having a dedicated CLI arg
Well, the dedicated CLI flag was there first, so we tried our best to keep it for backwards compatibility.
are we expecting controller debug message to show when running with --log-level debug?
I'm actually not sure. I'd have to go through the PR again to look it up. Either way, it seems broken, right?
There was a problem hiding this comment.
Either way, it seems broken, right?
My assumption from reading the --help output was that it wouldn't log controller related debug message if not explicitly opted-in so with that understanding, yes it's broken.
But we could also say the man is misleading and that --controller-debug will enable debug message only for controller, whereas --log-level debug enable all log messages, including controller ones.
IMO, it should be 1) - when I implemented the S4 Mk3 mapping, I add to add that nasty workaround, otherwise I would get a 10G+ logfile after an hour session, due to that message logging every single HID packet coming in every 20ms
There was a problem hiding this comment.
@Swiftb0y are you happy to keep this work around as-is and create a follow up issue + FIXME comment?
src/controllers/scripting/legacy/controllerscriptenginelegacy.cpp
Outdated
Show resolved
Hide resolved
acolombier
force-pushed
the
feat/improve-screen-rendering-framework
branch
from
c6fd897
to
99fed94
Compare
| QHash<QString, TransformScreenFrameFunction> m_transformScreenFrameFunctions; | ||
| // the QML root item. Note that the pointer is owned by the QML scene which | ||
| // will free them on shutdown (ControllerScriptEngineLegacy::shutdown) | ||
| QHash<QString, mixxx::qml::QmlMixxxControllerScreen*> m_rootItems; |
There was a problem hiding this comment.
lets avoid introducing manual memory management. You can wrap this in a unique_ptr. If that doesn't work because QHash needs the key to be copy-able, use std::unordered_map instead.
There was a problem hiding this comment.
Any reason why introducing a smart pointer on a Qt-owned reference here? Just in case you've missed the comment, the pointer is managed by the QML scene, we don't have any control over it.
There was a problem hiding this comment.
Ah, I just spotted it now. Well the problem is that we do call delete on it. So its very easy to mistake it as properly owning. I also fear that the pointer can easily outlive the RenderingEngine their tied to. Ideally the pointers would stay entirely in their rendering engine and in order to access them, you need to access the rendering engine.
There was a problem hiding this comment.
So sometimes you do delete them and sometimes you don't. It would be really nice if you could fix those ownership issues here.
There was a problem hiding this comment.
It looks liked I was wrong about the QML ownersphip
The ownership of the returned object instance is transferred to the caller.
So I will replace that Hash/fatPtr with unordered_map/unique_ptr
There was a problem hiding this comment.
ah, yeah. I was interpreting the
rootItem->setParentItem(pScreen->quickWindow()->contentItem());line as transferring ownership, but thats obviously wrong on second look.
This is a refactor largely inspired from the work in #13459 to simplify the hook system used for screen rendering.