Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change namespace and directory of osgKido #588

Merged
merged 10 commits into from
Jan 21, 2016

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Jan 20, 2016

This pull request changes the namespace and directory of osgKido to kido/gui/osg and kido::gui, respectively. Note that this pull request is targeting to package_reorganizing branch of #587.

@jslee02 jslee02 added this to the KIDO 0.1.0 milestone Jan 20, 2016
@jslee02 jslee02 mentioned this pull request Jan 20, 2016
@@ -69,7 +69,7 @@ if(MSVC)
else()
option(BUILD_SHARED_LIBS "Build shared libraries" ON)
endif()
option(KIDO_BUILD_OSGKIDO "Build osgKido library" ON)
option(KIDO_BUILD_OSGKIDO "Build osgKido library" ON) # TODO(JS): we probably should remove this option or add another build options for other extensions as well for consistency.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually do think we should have an option flag for each extension, although I don't consider that to be high priority.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, as KIDO 0.1.0 should be released as soon as possible, I'll add them for KIDO 1.0.0.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. This is only relevant to the build system anyway; it doesn't impact the packaging.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: Packing. The ideal way to package KIDO as a Debian would be to create one kido source package that Build-Depends on the build dependencies all components. The source package would build a separate binary package for each component (e.g. kido-optimizer-nlopt) that Depends only on the runtime dependencies for the headers/library that it contains.

I doubt it is possible to change this structure within one Debian release, since it changes the names (and number) of binary packages built from the source package.

Is this the plan? Or will we stick with a single, monolithic kido package for now?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I understand correctly, KIDO 0.1 is already doing exactly what you mentioned, and KIDO 0.1 and KIDO 1.0 will have same package structure.

Edit: You can check that the debian control file.

@mxgrey
Copy link
Member

mxgrey commented Jan 20, 2016

I'm wondering if we should make the namespace kido::osg instead of kido::gui, because I'm concerned about the likelihood of name collisions if we introduce another GUI extension. To go along with that, I suppose we would also want to name the library kido-osg instead of kido-gui-osg.

Using a kido::gui::osg namespace would also be an option, but I feel like that would be pointlessly many layers of namespace.

@jslee02
Copy link
Member Author

jslee02 commented Jan 20, 2016

I agree with you in the point of the name collisions (e.g., kido::gui::Viewer is too general) and also that kido-gui-osg is actually not dependent on kido-gui. However, I'm not sure which one is practically preferred way in these:

  • kido::osg
  • kido::gui::osg
  • kido::gui-osg
  • leave it as kido::gui and use prefix like OSGViewer as what we did for collision detectors (FCLCollisionDetector).

@psigen
Copy link
Collaborator

psigen commented Jan 20, 2016

I don't have a strong preference either way, other than supporting the refactor into the kido:: namespace. But I would point out a few things:

  • Even if there is no structural reason for kido::gui::osg, it is semantically much clearer for users of the package to figure out what the osg package is doing and if they need it. In addition, it means that the directory structure of the source code can be similarly distinct.
  • There are other GUIs to consider. One good example is @mkoval's RViz interface for DART, which could fit into a kido::gui::rviz namespace in the future.
  • I would be against the kido::gui-osg option, this is just adding non-standard naming for no good reason.
  • The OSGViewer option might be fine if there aren't many supporting classes/source files but it might get a bit hard to tell what's distinct in doxygen docs if a viewer needs a lot of helper classes and they all go into the kido::gui namespace.

@mxgrey
Copy link
Member

mxgrey commented Jan 20, 2016

kido::gui-osg wouldn't be an option since gui-osg would not be a valid namespace token.

I'd rather not append OSG to the front of each class name, because I feel that would add unnecessary clutter to the naming, and the primary motivation of using namespaces is to avoid that kind of C-style name mangling.

@jslee02
Copy link
Member Author

jslee02 commented Jan 20, 2016

I see. We have then kido::osg and kido::gui::osg. I would prefer kido::gui::osg with the reasons @psigen pointed out. If kido::gui::osg is too long then we can reduce it using namespace aliasing.

@mxgrey
Copy link
Member

mxgrey commented Jan 20, 2016

I would be okay with either kido::gui::osg or kido::osg. I feel a slight preference for kido::osg because

  1. It's independent of everything that is currently in the gui namespace
  2. It does both user interaction and rendering (which are currently split up into gui and renderer)
  3. It's briefer

But @psigen brings up some compelling reasons for preferring kido::gui::osg, so I would be okay with kido::gui::osg if others prefer it.

@psigen
Copy link
Collaborator

psigen commented Jan 20, 2016

I do agree with @mxgrey's points, but I think they might be better handled by subsequent cleanup of the gui and renderer namespaces -- my understanding is that there is a fair bit of legacy stuff in there that we might want to deprecate over time anyway.

The gui vs renderer namespacing is an interesting point. I'm not sure what to make of it: keeping them separate makes sense to account for offline-rendering and similar APIs, but many components will be implementing a tightly coupled renderer and some sort of GUI just like OSG.

@mxgrey
Copy link
Member

mxgrey commented Jan 20, 2016

If we decide to make it kido::gui::osg, then I would suggest that we deprecate the renderer namespace and refactor the existing rendering and gui codebase into its own subdirectory of gui. I think we're already planning on replacing that existing codebase with a GLFW (or something similar), so we can do this when we introduce that change.

@psigen
Copy link
Collaborator

psigen commented Jan 20, 2016

That's a good point, something like kido::gui::legacy or kido::gui::glfw would make a lot of sense to cleanup the existing codebase. Then kido::gui itself could be reserved for if we find common GUI-related interfaces and helper functions that solely depend on kido.

@jslee02
Copy link
Member Author

jslee02 commented Jan 20, 2016

All sounds good. The renderer dependency of dynamics (e.g., draw function) also should be removed in the work of renderer namespace deprecation so that the core library kido is independent on rendering and gui.

I will change the namespace kido-gui to kido::gui::osg in this pull request (since it can be done quickly), but the renderer namespace deprecation will be done in other pull request since it would need a lot more work and wouldn't affect on the package organization.

@mkoval
Copy link
Collaborator

mkoval commented Jan 20, 2016

👍 for removing the draw() function in kido::dynamics. I thought it was a bit odd to have there, since the implementation of that function is tied to a specific viewer.

@mxgrey
Copy link
Member

mxgrey commented Jan 20, 2016

That plan sounds good to me.

jslee02 added a commit that referenced this pull request Jan 21, 2016
@jslee02 jslee02 merged commit 1bb64a7 into package_reorganizing Jan 21, 2016
@jslee02 jslee02 deleted the package_reorganizing_osgKido_rename branch January 27, 2016 06:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants