Reorganize glut (preparation for glfw) #1088

jslee02 · 2018-07-19T05:58:29Z

This PR reorganizes glut. This is a preparation for having glfw, which will eventually replace glut since glut is deprecated.

This PR shouldn't hurt any existing code except it will generate warnings if glut is included by old files.

glfw support will be followed soon.

Before creating a pull request

Document new methods and classes
Format new code files using clang-format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md

codecov · 2018-07-20T05:51:16Z

Codecov Report

Merging #1088 into release-6.6 will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff              @@
##           release-6.6    #1088   +/-   ##
============================================
  Coverage        56.59%   56.59%           
============================================
  Files              320      320           
  Lines            24524    24524           
============================================
  Hits             13879    13879           
  Misses           10645    10645

mxgrey

I'll be pushing a few changes to this branch, if that's okay. Explanations are provided by inline comments.

mxgrey · 2018-07-27T09:28:35Z

dart/gui/GlutWindow.hpp


-#include "dart/gui/LoadOpengl.hpp"
-#include "dart/gui/RenderInterface.hpp"
+#warning "This file is deprecated in DART 6.6. "\


It seems this method for warning is not portable.

We might want to add a DART_PREPROCESSOR_WARNING(X) macro to dart/common/Deprecated.hpp in order to have a cross-platform method for this.

After some more thought, a preprocessor directive can't replaced with a macro because of the limitations of the preprocessor.

However, it seems recent versions of GCC and Clang can understand #pragma message, so we can use that instead.

mxgrey · 2018-07-27T09:29:08Z

dart/gui/GraphWindow.hpp

-#include <Eigen/Dense>
-
-#include "dart/gui/Win2D.hpp"
+#include "dart/gui/glut/GraphWindow.hpp"

 namespace dart {
 namespace gui {



We might want to add a DART_PREPROCESSOR_WARNING(X) to this header.

mxgrey · 2018-07-27T09:34:46Z

dart/gui/glut/GlutWindow.hpp

+namespace glut {
+
+/// \brief
+class Window {


Since the class name has changed to dart::gui::glut::Window, we should consider changing the filename to dart/gui/glut/Window.hpp instead of ../GlutWindow.hpp.

jslee02 · 2018-07-29T01:12:36Z

@mxgrey Thanks for the changes! Merging now.

Edit: This PR will go into 6.7 because I don't think the rest of changes for GUI won't be completed until 6.6 release.

mxgrey · 2018-08-02T02:18:17Z

dart/gui/CMakeLists.txt

@@ -62,6 +78,25 @@ add_subdirectory(osg)

 # Generate header for this namespace
 dart_get_filename_components(header_names "gui headers" ${hdrs})
+
+# Remove deprecated files from the list
+list(REMOVE_ITEM header_names


I stumbled across a potential issue with this. I think it's inadvertently breaking the API. If a user is depending on #include <dart/gui/gui.hpp> to get these headers, then their code will no longer compile.

We might want to consider allowing these to remain in the master header, even if it shoots compiler messages at people who are using the master gui header.

I think it's inadvertently breaking the API.

This is a valid point.

We might want to consider allowing these to remain in the master header, even if it shoots compiler messages at people who are using the master gui header.

I have a concern that the user will always see the warning messages by just including the master gui header. I can think of two possible solutions to this:

Remove header warnings completely
Easiest solution. The deprecated header will be removed in the next major release. The downside of this method is obviously that the user won't get a prior notice the header will be removed even when the header is included explicitly (not by the master header).

Suppress header warning when it's included by a master header
This would require some work for us. We could use a processor of whether the deprecated header is included by the master header, and suppress the warning if so. One caveat of this is that it's possible to unintentionally suppress the warning if a deprecated header is firstly included by the master header and then included explicitly. This is because the second header inclusion would be just ignored by the header guard. One workaround would be putting the warning out side of the header guard, but this would pollute the output window too much.

Maybe there is a much cleaner way, which I cannot think of now. Let me know.

I had exactly the same thoughts, plus one additional thought (which might not be the nicest):

If people are comfortable with using a "master" header, then they're probably not interested in best practices and wouldn't be particularly bothered by the compiler message. The master headers are provided for beginner convenience, but I don't think we should be encouraging their use in the first place. Although I suspect some of our examples and/or tutorials are probably using the master headers, so we'd need to do some work on our end to change that if we simply allow the compiler messages to print out.

I'd be fine with any of these three solutions.

Reorganize glut (preparation for glfw)

a5c4cec

jslee02 added this to the DART 6.6.0 milestone Jul 19, 2018

jslee02 requested a review from mxgrey July 19, 2018 05:58

Fix incorrect deprecation version

8a62cb2

jslee02 added the pr: work-in-progress label Jul 19, 2018

jslee02 and others added 5 commits July 19, 2018 20:20

Add more occupancy update functions to VoxelGridShape (#1083)

91080d3

Fix including deprecated header

7d7d7c3

Fix using deprecated APIs

b0d564e

Update changelog

a8b6c7b

Merge branch 'release-6.6' into enhance/glut_v3

30964d4

jslee02 removed the pr: work-in-progress label Jul 20, 2018

jslee02 and others added 2 commits July 19, 2018 21:06

Update license headers

65a6f31

Revert unintended change

0411d9c

Merge branch 'release-6.6' into enhance/glut_v3

f4862ce

jslee02 added the pr: work-in-progress label Jul 25, 2018

mxgrey requested changes Jul 27, 2018

View reviewed changes

mxgrey added 4 commits July 27, 2018 17:56

Use a more portable warning directive

8034eb0

Explicitly mark backwards compatibility aliases as deprecated

b99b10c

Make style consistent

dcde2dd

Rename glut/GlutWindow.* to glut/Window.*

0090427

mxgrey approved these changes Jul 27, 2018

View reviewed changes

jslee02 modified the milestones: DART 6.6.0, DART 6.7.0 Jul 27, 2018

jslee02 changed the base branch from release-6.6 to release-6.7 July 29, 2018 01:13

jslee02 merged commit 84538aa into release-6.7 Jul 29, 2018

jslee02 deleted the enhance/glut_v3 branch July 29, 2018 01:14

mxgrey reviewed Aug 2, 2018

View reviewed changes

jslee02 mentioned this pull request Aug 4, 2018

Consider adding deprecated headers to master headers #1092

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reorganize glut (preparation for glfw) #1088

Reorganize glut (preparation for glfw) #1088

jslee02 commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 20, 2018 •

edited

Loading

mxgrey left a comment

mxgrey Jul 27, 2018

mxgrey Jul 27, 2018

mxgrey Jul 27, 2018

mxgrey Jul 27, 2018

jslee02 commented Jul 29, 2018 •

edited

Loading

mxgrey Aug 2, 2018

jslee02 Aug 2, 2018

mxgrey Aug 3, 2018

Reorganize glut (preparation for glfw) #1088

Reorganize glut (preparation for glfw) #1088

Conversation

jslee02 commented Jul 19, 2018 • edited Loading

codecov bot commented Jul 20, 2018 • edited Loading

Codecov Report

mxgrey left a comment

Choose a reason for hiding this comment

mxgrey Jul 27, 2018

Choose a reason for hiding this comment

mxgrey Jul 27, 2018

Choose a reason for hiding this comment

mxgrey Jul 27, 2018

Choose a reason for hiding this comment

mxgrey Jul 27, 2018

Choose a reason for hiding this comment

jslee02 commented Jul 29, 2018 • edited Loading

mxgrey Aug 2, 2018

Choose a reason for hiding this comment

jslee02 Aug 2, 2018

Choose a reason for hiding this comment

mxgrey Aug 3, 2018

Choose a reason for hiding this comment

jslee02 commented Jul 19, 2018 •

edited

Loading

codecov bot commented Jul 20, 2018 •

edited

Loading

jslee02 commented Jul 29, 2018 •

edited

Loading