Feat/add abstractrenderview #58544
Feat/add abstractrenderview #58544
Conversation
There was a problem hiding this comment.
I like where this is heading, but I have various comments about the implementation...
QgsAbstractRenderView
- what is a render view - is it one render pass, or "one or more" render passes? in Qt 3D a render view is a single pass AFAIK, but in 3d axis implementation there are two passes in one render view...
- enableSubtree() - let's rename to setEnabled(bool) - the subtree enabler is an implementation detail
- constructor set "unnamed_view" as the default view name - I would suggest to remove the default name, IMHO we always want named views
- createSubtreeEnabled() - only used from abstract render view's constructor, let's put that code into the constractor to simplify things
- there are various methods which IMHO should not be in the API of the abstract base class - none of them seems to be essential for a render view definition, and they just leak some implementation details
- viewport() - only used by axes render view
- updateTargetOutputSize() - only used by shadow render view
- setRenderTargetOutput() - only used by shadow render view
- targetOutputs() - never used?
- outputTexture() - only used by shadow render view, it does not need to be QTexture2D elsewhere
- layerToFilter() - confusing what it really means (and e.g. axes render view has two layers internally). should be handled only in sub-classes.
- topGraphNode() - why is it virtual?
3d axis render view
- it adds an unused render target selector to one of the two internal render passes. why?
shadow render view
- the render view should have a setShadowSettings(const QgsShadowSettings&) that would be called from Qgs3DMapScene, rather than doing multiple calls to the render view
- I think we should not be adding signals for user-controlled settings to the render view (e.g. shadowRenderingEnabled() or shadowBiasChanged() ) - there's already a signal in Qgs3DMapSettings for that.
frame graph integration
- axes render view gets its cameras from outside, while shadow render view creates its camera. this should be consistent across render views. my preference: cameras, textures etc should be created and owned by render views (e.g. shadowMapTexture and shadowRenderTargetOutput should be in the shadow render view)
- render views are getting attached to main viewport - but 3d axis rendering was previously attached right to the frame graph's root node, it has its own viewport
- how are we going to handle order of render views? right now that's given only by the order how they are registered - is that going to stay, or will that change?
- in 3d axis object, there's a new mIsFrameGraphDestroyed member variable. this hints that there's some trouble with lifetime of frame graph vs its render views. Probably the frame graph should remove all the render views before deleting itself
- render views are registered with a name, but they are already given a name in constructor - this is a bit confusing, let's just have one name
- it is awkward to access stuff from render view using dynamic_cast<X*>( renderView(name)) when we know the render view always exists - let's just use direct shadowRenderView()
misc
- framegraph dump - given that there's a change of IDs in framegraph dumps, it is very difficult to compare the dumps before/after. how about we remove the IDs?
- throughout QGIS code it is not common to have #include "subdir/file.h" - we rather add the subdir to the list of include dirs
|
In Qt3D, one leaf node of the frame graph corresponds to one render pass, and it will create a (private) structure called a renderview. So what is commonly referred to as render pass is the same thing as a render view. But again, the term render view is essentially an implementation details of Qt3D. In this case here, a QSGRenderView may correspond to several actual passes, so not using the "pass" name seems appropriate. Maybe that name is clear enough for QGIS, or maybe it should be a RenderStep, RenderPhase... but might make things more confusing? |
|
@wonder-sk thanks for the review. There are many points!
ok lets go for RenderStage?
done
done
done
this is a function I added in anticipation for future RenderStage
it will be used by other RenderStage
it will be used by other RenderStage
it will be used by other RenderStage
it will be used by other RenderStage
This function allows you to retrieve the layer used to add entity to this RenderStage. I will change the comment.
indeed |
I am okay with the current naming (QgsAbstractRenderView). It was not clear to me whether a render view would be strictly one pass, or it could be multiple passes (i.e. whether the 3d axis implementation is fine or it should be split to two render views). As long as it is documented, it's fine. As for the other member functions in QgsAbstractRenderView, I would strongly suggest to add them only once they are really needed. The API surface of this base class should be as minimal as possible, which I guess boils down to providing a frame graph subtree. All the other bits like render outputs seem like stuff specific to some particular render views (and suitable for addition in derived classes). I think layerToFilter() is also not generic enough for the abstract render view class - e.g. what if I have a render view with three kinds of objects, each with a different layer to be applied? |
old remains ==> removed
ok but at first my intention was to extract the render views into dedicated files without changing the logic nor to fix legacy behaviours. But I am agree using
same as above
I am agree to uniformise the creation of objects but in my mind the render views could be created elsewhere or for other purposes, so cameras and textures should be created outside and set into the render views as needed.
IMHO, the framegraph role is to manage the different views. In a near future the render view creation / activation / order will be more programmable. For now the framegraph still creates the render views in its constructor as previously.
As I remember, there was some kind a object destruction concurrency that generate segfault.
agree, I add the name in the renderview name the latter and it may need a bit a refactoring to be less confusing
render view always exists - let's just use direct shadowRenderView()
For now the ID are dumped according to a reference this allows to have the same dump from Qt5 and Qt6. |
benoitdm-oslandia
force-pushed
the
feat/add_abstractrenderview
branch
from
44bcf90
to
324e670
Compare
|
hi! I have applied the requested fixed and to avoid a very large number of commit I rebased and force pushed |
…s as render view top nodes Co-authored-By: Mike Krus <[email protected]>
* add renderview names
…view When the project is saved in an old version, minViewportRatio or maxViewportRatio can have bad saved data (ie. 0 values) leading to always disable axis view.
…e into render view
benoitdm-oslandia
force-pushed
the
feat/add_abstractrenderview
branch
from
4dd2973
to
93e0b66
Compare
|
rebase against master to fix conflict. @nyalldawson @wonder-sk is that ok for you? |
|
As there are no more comments or requests, @lbartoletti can you merge this PR please? |
| //! Set output to screen (ie. nullptr) or to a render target output | ||
| virtual void setTargetOutputs( const QList<Qt3DRender::QRenderTargetOutput *> &targetOutputList ); | ||
|
|
||
| //! Returns the list of all target outputs | ||
| virtual QList<Qt3DRender::QRenderTargetOutput *> targetOutputs() const; | ||
|
|
||
| //! Updates texture sizes for all target outputs | ||
| virtual void updateTargetOutputSize( int width, int height ); | ||
|
|
||
| //! Returns the 2D texture attached at the \a attachment point, if any | ||
| virtual Qt3DRender::QTexture2D *outputTexture( Qt3DRender::QRenderTargetOutput::AttachmentPoint attachment ) const; | ||
|
|
||
| //! Returns the layer to be used by entities to be included in this render view | ||
| Qt3DRender::QLayer *layerToFilter() const; | ||
|
|
||
| //! Returns the viewport associated to this render view | ||
| virtual Qt3DRender::QViewport *viewport() const; |
There was a problem hiding this comment.
As highlighted in my earlier review, I do not see why any of these functions should be in the abstract render view interface. It is unclear to me why and when some future render view implementations should be using them. Please let's make the abstract render view interface minimal: it should have a frame graph sub-tree, a name, and enabled flag. Everything else is IMHO stuff that should be handled in subclasses.
- targetOutputs() - anyone who need a list of target outputs could get them by walking the render view's sub-tree and check for QRenderTargetSelector
- setTargetOutputs() - unclear what should this do - a render view may have multiple steps, each with different target outputs... what is supposed to happen when this function would be called?
- updateTargetOutputSize() - I can imagine a function like this in the interface, but probably named differently, to mean that the window size is going to be different, and the render view should react to that (there may be various target outputs, some with sizes independent from the window size)
- outputTexture() - unclear why this should not be handled elsewhere. There could be multiple render passes, with different output textures attached at different passes, so this is not well defined
- layerToFilter() - there may be multiple layers used for different purposes (already the case for axis render view), not sure why the render view should return exactly one layer
- viewport() - some render views may use the "default" viewport, others may use its custom viewport, or each render pass of a render view may have a different viewport - so it's unclear what a multi-pass render view should return
If there are issues with the definition of these functions, it is a sign that they should be changed, or removed from the interface. I would vote for their removal and introduce any functions if it is very clear they are generic enough.
There was a problem hiding this comment.
Thanks @wonder-sk for the review!
Sorry for the confusion, I thought I had answered your questions previsiously. As I said this PR is the first of a series, the next PR will provide the other render view extractions. If you like, you can check how the final framegraph and renderviews will look like on this branch. I hope the use of these functions will be clearer.
And I am agree the viewport() function is to specific but I does no agree with the others. By reading your comments I understand the functions names or/and comments could be improved. I will correct this asap.
There was a problem hiding this comment.
I am looking at the linked branch, but I still fail to understand why we need Qt3DRender::QRenderTargetSelector *mRenderTargetSelector member in QgsAbstractRenderView and all the functions for rendering targets in the base class. Why do they need to be there? Why can't render passes define their target selectors as needed? As noted above, I can imagine that we would have e.g. handleWindowResize(QSize size) so that render views can update their texture sizes if they depend on the window size, but I am unsure about all the other ones.
Same with layerToFilter() - each render view should provide its layer(s) for filtering if needed in their derived class, and so a render view could e.g. expose multiple layers for different occasions, or maybe no layers at all.
There was a problem hiding this comment.
I am looking at the linked branch, but I still fail to understand why we need
Qt3DRender::QRenderTargetSelector *mRenderTargetSelectormember inQgsAbstractRenderViewand all the functions for rendering targets in the base class. Why do they need to be there? Why can't render passes define their target selectors as needed? As noted above, I can imagine that we would have e.g.handleWindowResize(QSize size)so that render views can update their texture sizes if they depend on the window size, but I am unsure about all the other ones.
The goal here is to go further than simply handling resizing. This idea is to separate the responsibility between the texture creation and their usage to: ease their accesses, sets, updates size. Also, by having the mRenderTargetSelector in the abstract class we match the existing renderviewes : they all have one mRenderTargetSelector.
Same with layerToFilter() - each render view should provide its layer(s) for filtering if needed in their derived class, and so a render view could e.g. expose multiple layers for different occasions, or maybe no layers at all.
ok it is possible to do without them, I will remove the layerToFilter() and viewport function.
There was a problem hiding this comment.
The goal here is to go further than simply handling resizing. This idea is to separate the responsibility between the texture creation and their usage to: ease their accesses, sets, updates size.
I am not sure I understand. Could you please expand your thoughts, and how that relates to having Qt3DRender::QRenderTargetSelector *mRenderTargetSelector in QgsAbstractRenderView ?
Also, by having the mRenderTargetSelector in the abstract class we match the existing renderviewes : they all have one mRenderTargetSelector.
Well, not necessarily... How about ambient occlusion render view (in your branch) - it has two render target selectors - which one is the one?
My point is that we should expect arbitrary sub-graph in a single render view, and not have some frame graph nodes treated specially in the base class. If there would be always exactly one render target selector in every render view, then it would be fine, but I don't see that here.
I will remove the layerToFilter() and viewport function.
Thanks!
There was a problem hiding this comment.
I am not sure I understand. Could you please expand your thoughts, and how that relates to having Qt3DRender::QRenderTargetSelector *mRenderTargetSelector in QgsAbstractRenderView ?
because you need a QRenderTargetSelector to be able to add QRenderTarget to handle textures. Indeed if a renderview does not need to write into textures, it will not need all these helping functions. But in that case the instantiated object will only contain one empty pointer to the mRenderTargetSelector.
Well, not necessarily... How about ambient occlusion render view (in your branch) - it has two render target selectors - which one is the one?
The one in which you want to write the final result. The other one is just for a temporary one.
I got your point but at the beginning I had made nearly the same design you want to, but I had a lot of duplicate code between the different renderviews. In order to find a compromise, I can provide 2 abstract classes, one really basic and another one extending the first with the output texture helper functions.
There was a problem hiding this comment.
I have checked the code on your branch again (master...benoitdm-oslandia:QGIS:feature/extendable-framegraph) and I still struggle to understand why they should be there - they make the abstract class more complicated, they are not generic enough, and the code is less readable. I don't see where you would get a lot of duplicate code either...
Just to make sure we're on the same page, here's what I am suggesting:
- remove setTargetOutputs(), targetOutputs(), outputTexture(), onTargetOutputUpdate() functions and mRenderTargetSelector, mTargetOutputs member variables from QgsAbstractRenderView class
- rename updateTargetOutputSize(QSize) to handleWindowResize(QSize) and leave its default implementation empty
- using the forward render view as an example:
- QgsForwardRenderView constructor
- creation of textures and render outputs should be done here (not in QgsFrameGraph::constructForwardRenderPass())
- it will create QRenderTarget, assign render outputs, create QRenderTargetSelector and assign it
- QgsForwardRenderView should get two new public non-virtual functions:
QTexture2D* colorTexture()QTexture2D* depthTexture()
- QgsForwardRenderView would implement handleWindowResize() and set texture size of its color+depth textures
- QgsFrameGraph::constructForwardRenderPass()
- the whole function body would end up being just:
registerRenderView( new QgsForwardRenderView( this, mMainCamera ), FORWARD_RENDERVIEW );
- the whole function body would end up being just:
- QgsForwardRenderView constructor
This is IMHO much, much easier to read and understand - because the whole frame graph fragment is built in one place together with any other objects - rather than having a bunch of virtual methods that only make sense in some contexts.
There was a problem hiding this comment.
I don't quite agree with your suggestions because there is no longer any use for this abstract class but I am not in a position to refuse if I want this work to move forward. So I will make these corrections.
| Qt3DRender::QFrameGraphNode *topGraphNode() const; | ||
|
|
||
| //! Enable or disable via \a enable the render view sub tree | ||
| virtual void setEnabled( bool enable ); |
There was a problem hiding this comment.
One thought I had recently about enabling/disabling render views - is it correct to have the chance to enable/disable any of them? I understand it is useful to disable e.g. shadow rendering or 3d axis or ambient occlusion views, but how about things like the forward pass or the post-processing pass - does it make sense to ever disable them? I wonder whether it would be a good idea to have the enable/disable functionality covered in a subclass (e.g. QgsOptionalRenderView) that would add setEnabled()/isEnabled() functions - or maybe have virtual isRequired() function in QgsAbstractRenderView which would determine behaviour of setEnabled(false) ?
What are your thoughts?
There was a problem hiding this comment.
Hi @wonder-sk, I would prefer to add a isRequired function I would be clearer in my POV!
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
github-actions
bot
added
the
stale
This PR is part of qgis/QGIS-Enhancement-Proposals#259 QEP (relates to qgis/QGIS-Enhancement-Proposals#252).
It introduces the
AbstractRenderviewconcept to split the framegraph into a more modular way.As many files will be introduced, we move all framegraph related files in a new subdirectory
framegraph.As examples, we extracted 2 renderviewes: one for the 3dAxis and one for the shadows.
cc @ptitjano @mkrus
Funded by CEA/DAM @renardf