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

Expose shadow texture size for directional lighting in SDF #633

Merged
merged 12 commits into from
Aug 21, 2024

Conversation

athenaz2
Copy link
Contributor

🎉 New feature

Related to gazebosim/gz-rendering#1034, view that for details.

Summary

Test it

Checklist

  • Signed all commits for DCO
  • Added tests
  • Added example and/or tutorial
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

@github-actions github-actions bot added the 🏛️ ionic Gazebo Ionic label Aug 13, 2024
@athenaz2
Copy link
Contributor Author

athenaz2 commented Aug 13, 2024

Only recently realized that my PR breaks ABI, so PR is for main. I've been working with the default gz-gui, gz-rendering branches as the base until now. Will test my changes (to both gz-gui, gz-rendering) with main branch asap.

EDIT - my changes in both gz-gui, gz-rendering work in main branch, which is good

…false; update names of enum vals used

Signed-off-by: Athena Z <[email protected]>
Signed-off-by: Athena Z <[email protected]>
@jennuine jennuine requested a review from iche033 August 16, 2024 18:09
@jennuine
Copy link
Contributor

@iche033 if you have some bandwidth, would you be able to review this since you already have some context with gazebosim/gz-rendering#1034?

@iche033
Copy link
Contributor

iche033 commented Aug 16, 2024

@iche033 if you have some bandwidth, would you be able to review this since you already have some context with gazebosim/gz-rendering#1034?

yep will do

Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

just some minor comments on error msgs

src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
@@ -238,6 +239,8 @@ namespace gz::gui::plugins
/// \brief True if sky is enabled;
public: bool skyEnable = false;

public: unsigned int directionalLightTextureSize = 2048u;
Copy link
Contributor

Choose a reason for hiding this comment

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

add doxygen comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops. Added here 0bc87f8

@@ -368,6 +371,12 @@ namespace gz::gui::plugins
/// \param[in] _sky True to enable the sky, false otherwise.
public: void SetSkyEnabled(const bool &_sky);

/// @brief \brief Set the shadow texture size for the given light type.
/// @param _lightType Light type that creates the shadow
/// @param _textureSize Shadow texture size
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: replace @ with \ for consistency

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/plugins/minimal_scene/MinimalScene.hh Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
src/plugins/minimal_scene/MinimalScene.cc Outdated Show resolved Hide resolved
athenaz2 and others added 2 commits August 16, 2024 13:52
@azeey azeey added the beta Targeting beta release of upcoming collection label Aug 16, 2024
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

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

Looks good to me

Just a note that this works for the 3D scene in the GUI. In the future, we may want to extend this functionality to shadows on the server side for sensors.

@iche033 iche033 merged commit 96098d6 into gazebosim:main Aug 21, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta Targeting beta release of upcoming collection 🏛️ ionic Gazebo Ionic
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants