Not to stir up any long discussions, but how about vtkQWidget? I always found the QVTK prefix a little off from normal VTK class naming.
May be out of the question considering the other QVTK-prefixed helper/implementation classes, but thought I’d air the idea at least.
The reasoning behind the QVTK prefix was that it indicated to the user/developer that it is a QObject subclass. Also, the API style would follow Qt’s rather than VTK’s, thus methods named with lowerCase etc etc.
QVTKOpenGLWidget sounds good, but usually the “OpenGL” word is not included in class names that users should normally use (only in low-level implementation classes). So, QVTKWidget, QVTKRenderWidget, QVTKRenderWindowWidget names would be somewhat more appropriate.
Yep, I understand the logic. vtk* ==> typically inherits vtkObject/vtkObjectBase, QVTK ==> inherits QObject. I believe it’s discouraged to use the Q* prefix for classes outside of Qt itself though (even if the VTK probably makes this OK).
In my suggestion vtkQWidget, I thought that even if it’s not inheriting vtkObject, it is a VTK class, so has the vtk* prefix, and it’s a QWidget, hence the QWidget in the name (the Q included to not be confused with VTK’s own widgets), and the OpenGL has been left out since it’s an implementation detail.
But the more I think of it, it’s probably water under the bridge, with the QVTK* prefix used for a bunch of classes in VTK at this point. So let’s keep it that way!
The OpenGL part is here to not confuse with the old QVTKWidget that as not been removed yet.
Maybe it is time to remove it, but we can’t use this name either.
Also, let’s not use the WIndow term, as it is heavily charged in Qt.
For QVTKOpenGLWidget :
For QVTKOpenGLNativeWidget :
Any other suggestions ? Which suggestions would you prefer ?
Let’s make a poll for this to make it easier to summarize the results:
For QVTKOpenGLWidget:
0 voters
For QVTKOpenGLNativeWidget:
0 voters
(damn, discourse is awesome 
)
Since we derive from QOpenGLWidget, we should definitely keep OpenGL in the name.
At some point, I expect that we will have an OpenGL and a Vulkan rendering backend and we will have difficulty to differentiate them without OpenGL and Vulkan prefixes.
+1
At some point we could have higher-level classes that dispatch to the right implementation (such as vtkRenderer etc.), but since we aren’t there yet but dealing directly with the OpenGL implementation, including OpenGL is okay and even necessary in my opinion.
Also, VTK class names usually read left-to-right from more specific to less specific (e.g. vtkOpenGLRenderWindow vs. vtkRenderWindow, vtkOpenGLPolyDataMapper vs. vtkPolyDataMapper, etc.). In keeping with that convention, I would vote for none of the options above but instead propose
QVTKStereoOpenGLWidget and QVTKOpenGLWidget
Same for me, those 2 names are simple and clear, I don’t like the other propositions
VTK users don’t care what underlying technology (OpenGL, OpenGLES, Vulkan, Metal, etc.) should be used to create a VTK render window in a Qt widget. Would you really want to create #ifdefs around instantiation of the render window in all Qt VTK tests, examples, and applications?
If you want, you can still expose specialized classes that allow users to choose a specific technology, but the QVTKRenderWidget should just work, with the best underlying available technology automatically selected.
That seems like a desirable goal, but that is beyond a simple renaming of the existing classes. The question at hand is what to rename these very OpenGL-specific widgets. They both rely on a vtkGenericOpenGLRenderWindow internally.
If you want to add that implementation-agnostic level above this now and advise people to use that, I have no objection. But as for a simple rename operation of these classes, I am arguing OpenGL should stay in the name.
I fully agree, we should have low-level classes that have OpenGL in their names. We should just not advertise to users that these low-level classes exist (don’t use in any examples and minimize their usage in tests) because we know that in a few years we will have alternative implementations.
It is about the same as with vtkActor / vtkOpenGLActor. Only advanced users should instantiate vtkOpenGLActor explicitly, everyone else should just use vtkActor and rely on VTK factory (or any other suitable mechanism) to instantiate the best implementation.
We should add the new implementation-agnostic classes at the same time we rename the existing classes. This allows users to switch to the final class names in one step instead of renaming classes in tests, examples, user code now and again in 1-2 years. Yes, it may require 1-2 extra days of work for a VTK developer now, but it may save an hour or more for hundreds of VTK-based projects.
Right now the poll is mixing both names for a high-level factory mechanism and an implementation-specific renaming, and that I think is confusing matters.
We should first get consensus that a factory mechanism/generic name for these widgets is the way to go, then figure out what their names should be. Agreed?
I agree. So let’s poll again :
0 voters
please refrain to post until this poll is closed in two or three days.
Looks like it is quite even.
After discussing with @cory.quammen, I think the best would be to :
The only low-level rename proposition was then :
QVTKOpenGLNativeWidget -> QVTKOpenGLNativeWidget (no rename)
QVTKOpenGLWidget -> QVTKOpenGLStereoWidget
Let me know if there is a strong opposition to this suggestion.
It should also be noted that QVTKOpenGLNativeWidget has an issue when enabling multisampling, see https://gitlab.kitware.com/vtk/vtk/issues/17154. The problem seems to come from Qt as understood by @utkarshayachit (https://gitlab.kitware.com/vtk/vtk/issues/17154#note_546700). Because of this, I’m using mostly QVTKOpenGLWidget in my own applications, which does not suffer from this issue. So personally, I’m not particularly in favor of renaming this widget by adding a “Stereo” label to it, since I don’t use QVTKOpenGLWidget for its stereo capabilities at all. But I agree that for the other widget, the “Native” term is not very clear.
The MR has been merged.