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.

Sign up for GitHub

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

Jump to bottom

Added renderOrder to convert functions #514

Merged
merged 5 commits into from
Jan 26, 2021
Merged

Added renderOrder to convert functions #514

merged 5 commits into from
Jan 26, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Dec 23, 2020

This PR is part of the effort to avoid z-fighting between coplanar meshes.

Related with gazebosim/gz-rendering#188

Signed-off-by: ahcorde [email protected]

@ahcorde ahcorde mentioned this pull request Dec 23, 2020
Merged
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Dec 23, 2020
chapulina
chapulina suggested changes Dec 23, 2020
View reviewed changes
src/Conversions.cc Show resolved Hide resolved
@chapulina chapulina added the needs upstream release Blocked by a release of an upstream library label Dec 23, 2020
@ahcorde ahcorde changed the base branch from ign-gazebo4 to main December 23, 2020 21:08
@ahcorde
Copy link
Contributor Author

ahcorde commented Dec 23, 2020

Retargeted to main

@chapulina
Copy link
Contributor

This is blocked by upgrading ign-gazebo5 to use ign-msgs7: gazebo-tooling/release-tools#362

@chapulina chapulina added 🏢 edifice Ignition Edifice and removed 🔮 dome Ignition Dome labels Jan 5, 2021
chapulina
chapulina reviewed Jan 5, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Works for me. Mind setting and checking the render order on some tests in Conversions_TEST?

chapulina
chapulina approved these changes Jan 5, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

Approved, but it can't be merged until the upgrade to ign-msgs7 is complete.

@chapulina
Copy link
Contributor

The ign-msgs7 upgrade is in, but now we need an SDF10 release: gazebosim/sdformat#461

chapulina
chapulina suggested changes Jan 25, 2021
View reviewed changes
src/Conversions_TEST.cc Outdated Show resolved Hide resolved
@ahcorde
fixed conversion tests
999ecdf 
Signed-off-by: ahcorde <[email protected]>
@codecov
Copy link

codecov bot commented Jan 25, 2021 •
edited
Loading

Codecov Report

Merging #514 (999ecdf) into main (63a88dc) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #514      +/-   ##
==========================================
+ Coverage   77.33%   77.38%   +0.05%     
==========================================
  Files         213      213              
  Lines       11951    11953       +2     
==========================================
+ Hits         9242     9250       +8     
+ Misses       2709     2703       -6
Impacted Files Coverage Δ
src/Conversions.cc 82.24% <100.00%> (+0.04%) ⬆️
src/SimulationRunner.cc 94.04% <0.00%> (+1.11%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 63a88dc...999ecdf. Read the comment docs.

@chapulina chapulina mentioned this pull request Jan 25, 2021
Merged
chapulina
chapulina approved these changes Jan 25, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

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

LGTM, just needs the homebrew bottle to be finished to fix CI: osrf/homebrew-simulation#1307

@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 26, 2021

@osrf-jenkins retest this please

@ahcorde
Copy link
Contributor Author

ahcorde commented Jan 26, 2021

This failing test is unrelated ServerRepeat/SceneBroadcasterTest.State to this PR. Merging

@ahcorde ahcorde merged commit ecc591c into main Jan 26, 2021
@ahcorde ahcorde deleted the ahcorde/renderOrder branch January 26, 2021 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iche033 iche033 iche033 approved these changes

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🏢 edifice Ignition Edifice needs upstream release Blocked by a release of an upstream library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ahcorde @chapulina @iche033