[Development] Some Qt3D feedback

[Sean Harmer]

Hey Steve,

thanks for the feedback!

On Monday 08 Jun 2015 01:11:23 Stephen Kelly wrote:
> Hello,
> 
> Congrats to Paul, Sean and others working on getting this module in a
> releasable state for Qt 5.5!
> 
> I have not reviewed the code, but I found some items to raise:
> 
> 1) The include/Qt3DCore/Window file doesn't have a Q prefix.
> 
> as every other header does. Should probably be Qt3DWindow.

Right, this actually needs removing and something temporary putting in place 
in the examples for now. The window class doesn't do much at all but there is 
a need for more full featured integration points. My current thoughts on what 
we should aim for are:

1) A convenience window to be used with Qt3D that has a standard camera and 
handles aspect ratio changes etc and also uses a default framegraph (that can 
be changed by the user). QML and C++ versions.

2) Qt3D Widget to embed a Qt3D scene into a QWidget hierarchy. Likely 
analogous to QOpenGLWidget but with the above Qt3D plumbing.

3) Embed Qt3D into a Qt Quick 2 scene. Already available via the Scene3D Qt 
Quick 2 element (provided by Qt3D module).

4) Use Qt Quick within Qt3D as e.g. a texture provider so that it is possible 
to use a Qt Quick 2 render within the 3D world.

I'll try to tidy up and move the existing Window thing to the examples later 
today or tomorrow.

> 2) A private header is included in a public header:
> 
>  include/Qt3DCore$ grep private/ *.h
>  qaspectjobmanager.h:#include <Qt3DCore/private/qt3dcore_global_p.h>
> 
> This is concerning - Don't we have a unit test preventing that?

I can't find that. Is that from the 5.5 branch?

> 3) The cmake unit tests don't pass.
> 
> It is easily fixable, but does this mean that the cmake tests are not run
> for this module? That is concerning.
> 
> Are unit tests run for this module in CI at all?

Hmm I thought they were, but obviously not. I'm looking at making the test 
work now.

> 4) Private dependencies
> 
>  git grep -w -e QT --and -e private
> 
> shows a bunch of content. Shouldn't they be added to QT_PRIVATE instead?

Right, https://codereview.qt-project.org/#/c/113899/

> 5) Qt3D namespace
> 
> This is the first time that all classes in a library are in a namespace.
> Previously only enums (in various modules) and free functions (in
> QtConcurrent) have been put in namespaces.
> 
> In QtConcurrent, the module name also appears in the header file, but that
> is not followed by Qt3D libraries.
> 
> Given that Qt has never put classes in a namespace like this, is there
> something to be consistent about here?
> 
> 6) QParameter is a very generic name
> 
> I realise it is in a namespace, but still...
> 
> Qt3DParamter might be better *and* more consistent. Similar applies to other
> classes.

It's precisely because of these kinds of issues that we decided to use 
namespaces in Qt3D rather than the poor-man's prefix name spacing. 

If it's required to not use namespaces to be part of the Qt project then we 
can of course change it. However, I would argue against doing so, especially 
in the light of being able to use some more modern C++ features in upcoming QT 
versions. Name spaces are supported everywhere these days so why not just use 
them, especially in a new add-on module?

> 7) Unneeded Q_DECLARE_METATYPE
> 
> Using the macro for QObject derived types is not needed. I saw
> 
>  Q_DECLARE_METATYPE(Qt3D::QNode *)
> 
> and similar for QParameter and then stopped searching.

Thanks, https://codereview.qt-project.org/#/c/113906/

> 8) Docs in headers.
> 
> I saw one method documented in qparameter.h instead of in the cpp and I
> didn't look for more.

The docs need a lot of work. I have some time set aside for doc writing this 
week and will do a clean up pass for such issues.

Thanks again,

Sean

-- 
Dr Sean Harmer | sean.harmer at kdab.com | Managing Director UK
KDAB (UK) Ltd, a KDAB Group company
Tel. +44 (0)1625 809908; Sweden (HQ) +46-563-540090
Mobile: +44 (0)7545 140604
KDAB - Qt Experts