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

ViewScene config as pop-up widgets #2318

Merged
merged 80 commits into from
Aug 18, 2021
Merged

ViewScene config as pop-up widgets #2318

merged 80 commits into from
Aug 18, 2021

Conversation

dcwhite
Copy link
Member

@dcwhite dcwhite commented Jul 28, 2021

The ViewScene configuration window has been broken up into many small widgets that pop-up on hover/click of the now expanded ViewScene toolbar. The color GUI has also been much improved.

Additionally, the first step was taken to convert boost smart pointers to std smart pointers, by updating a typedef. And the C++ standard was set using modern CMake to C++17, which will obviate a few older compilers that a few users might have still been using.

@dcwhite dcwhite requested a review from nids2001 July 28, 2021 20:22
@dcwhite dcwhite self-assigned this Jul 28, 2021
@dcwhite
Copy link
Member Author

dcwhite commented Jul 29, 2021

Forgot to mention the new code uses some of https://github.com/commontk/CTK/, thanks to @jcfr for introducing it.

@jessdtate
Copy link
Contributor

I have a couple suggestions:

  • I think the blank space in the top left corner between the icons is visually awkward. Could we move one of the toolbars to the corner?
  • the light positions, mostly the head light, could use a reset button since the it's difficult to set the precise position in the UI. the reset could also include the color if that's easier. Alternatively, a combo box with the position value would work.
  • Autorotate could use a stop button (in the middle), or by clicking on the same direction

@dcwhite
Copy link
Member Author

dcwhite commented Jul 30, 2021

Autorotate stop/start is on the main button (the one with the icon)

The rest I will add for sure!

@dcwhite dcwhite marked this pull request as draft August 5, 2021 06:32
@jcfr
Copy link

jcfr commented Aug 5, 2021

Few preliminary nitpicks:

  • changes related to c++ standard, use of shared pointer should be done in a dedicated PR
  • integration of CTK should be fixed to work using external project instead of copying the sources (idea is to avoid src/Interface/Modules/Base/CustomWidgets/CTK/Core/ctkValueProxy.h)

@dcwhite
Copy link
Member Author

dcwhite commented Aug 5, 2021

I agree with both of those points. Reasoning behind the opposite decision: Our PR-review rate is extremely slow, so I tend to lump features and std-refactoring together. Ideally, I would submit multiple PRs on different branches, but if I did that the PRs would stack up even higher with likely a backlog of a year or more. Also, I tried setting up CTK as an external project, we have the cmake machinery for that, but I ran into several problems, which I've forgotten, so this is a "CTK-lite" attempt to just get a few widgets copied in. Normally I wouldn't do that either, but I was in a rush to start using them. See Qwt as an external--I've had many problems with that one over the years, and opted for fast-and-messy for this dependency.

@dcwhite dcwhite marked this pull request as ready for review August 8, 2021 19:38
@dcwhite dcwhite merged commit d9f809c into master Aug 18, 2021
@dcwhite dcwhite deleted the ctk+shared_ptr branch August 18, 2021 15:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants