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

Small cleaning of Magnum GUI #47

Merged
merged 7 commits into from
May 13, 2020
Merged

Small cleaning of Magnum GUI #47

merged 7 commits into from
May 13, 2020

Conversation

costashatz
Copy link
Member

@costashatz costashatz commented May 11, 2020

This PR cleans-up the Magnum GUI.. This is still work in progress I have still to:

  • Follow the robot_dart style everywhere
  • Expose all the functionality of BaseApplication to Graphics class
  • See if I can reduce the duplicate code (e.g., calling functions of BaseApplication in Graphics)

To speed-up the graphics rate I will open a different PR.

@costashatz costashatz added the WIP label May 11, 2020
@costashatz costashatz removed the WIP label May 11, 2020
@costashatz costashatz self-assigned this May 11, 2020
@costashatz
Copy link
Member Author

@jbmouret @Aneoshun This PR is breaking the API of Magnum::Graphics: among other things and cleaning, I added a GraphicsConfiguration struct as an argument to the constructor instead of the many many arguments (this is the only actual API breakage; the others are internal and should not affect any of your usages). This will enable us to more easily not break the API the next time we would like to add an argument and makes it easier to write/handle default parameters. Can you verify the following?

  • That it compiles on your setups and you can run the examples?
  • That the behavior of the lights/shadows is as it was before.
  • That you are OK to merge (meaning you acknowledge the breaking of the API to adapt your projects)?

Thanks a lot!

@costashatz costashatz added this to the RobotDART 1.0.0 milestone May 12, 2020
@jbmouret
Copy link
Collaborator

@costashatz Yes, everything works fine for me.

As we are not using the graphics API right now, I am OK with merging (this is the time for cleaning things).

@costashatz
Copy link
Member Author

@costashatz Yes, everything works fine for me.

Great! I'll wait a few days for @Aneoshun..

Pinging also @ErickKramer as he was using the library.

@Aneoshun
Copy link
Contributor

Hi @costashatz,

I am pinging my student @Lookatator as he is using this API every day. He will be better suited than me to comment on this PR.

@ErickKramer
Copy link

Hi @costashatz
Thank you for pinging me. I have no issue with the cleaning. I finished the work I was doing with the library. Thank you for all the help provided.

@Lookatator
Copy link

Hi @costashatz

It works perfectly for me as well. I am totally OK with merging 😃

@costashatz
Copy link
Member Author

Merging..

@costashatz costashatz merged commit 14a1171 into master May 13, 2020
@costashatz costashatz deleted the gui_fixes branch May 13, 2020 18:14
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.

5 participants