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

3 -> 4 #347

Merged
merged 8 commits into from
Jun 18, 2021
Merged

3 -> 4 #347

merged 8 commits into from
Jun 18, 2021

Conversation

iche033
Copy link
Contributor

@iche033 iche033 commented Jun 17, 2021

➡️ Forward port

Port ign-rendering3 to ign-rendering4

Branch comparison: ign-rendering4...ign-rendering3

Note to maintainers: Remember to Merge with commit (not squash-merge or rebase)

darksylinc and others added 7 commits June 14, 2021 13:43
@darksylinc @iche033
Fix floating point precision bug handling alpha channel (#332) (#333)
7613608 
Fixes #332
Fixes #108

Signed-off-by: Matias N. Goldberg <[email protected]>
@iche033
Fix depth alpha (#316)
b227f1e 
* update test

Signed-off-by: Ian Chen <[email protected]>

* reenable macos test

Signed-off-by: Ian Chen <[email protected]>

* fix typo

Signed-off-by: Ian Chen <[email protected]>
@iche033
cherry pick f9f1820 and fix conflicts
fe96179 
Signed-off-by: Ian Chen <[email protected]>
@iche033 @darksylinc
Backport memory fixes found by ASAN (#340)
a56c834 
* Fix heap overflow when reading (#337)

PF_RGB is 3 bytes. But later on Ogre2SelectionBuffer::OnSelectionClick
will try to read 4 bytes from it.

Fixed by ensuring it's always at least 4 bytes and zero-initializing
those 4 bytes.

Signed-off-by: Matias N. Goldberg <[email protected]>

Co-authored-by: Ian Chen <[email protected]>

* Fix new [] / delete mismatch (#338)

Using a custom deallocator to avoid breaking ABI

Alternatively C++17 supports the following syntax, which was not used:

`typedef std::shared_ptr<unsigned char[]> DataPtr;`

Signed-off-by: Matias N. Goldberg <[email protected]>

Co-authored-by: darksylinc <[email protected]>
@iche033
recreate node only when needed (#342)
fa71fc3 
Signed-off-by: Ian Chen <[email protected]>
@iche033
relax gaussian test tolerance (#344)
5164539 
Signed-off-by: Ian Chen <[email protected]>
@iche033
merge from ign-rendering3
7790c0b 
Signed-off-by: Ian Chen <[email protected]>
@github-actions github-actions bot added the 🔮 dome Ignition Dome label Jun 17, 2021
@codecov
Copy link

codecov bot commented Jun 17, 2021 •
edited
Loading

Codecov Report

Merging #347 (cce8431) into ign-rendering4 (97891ac) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head cce8431 differs from pull request most recent head 0551b74. Consider uploading reports for the commit 0551b74 to get more accurate results
Impacted file tree graph

@@                Coverage Diff                 @@
##           ign-rendering4     #347      +/-   ##
==================================================
+ Coverage           55.82%   55.83%   +0.01%     
==================================================
  Files                 147      147              
  Lines               13930    13935       +5     
==================================================
+ Hits                 7776     7781       +5     
  Misses               6154     6154
Impacted Files Coverage Δ
ogre2/src/Ogre2RenderTarget.cc 73.51% <100.00%> (ø)
ogre2/src/Ogre2SelectionBuffer.cc 79.68% <100.00%> (+0.32%) ⬆️
src/Image.cc 84.61% <100.00%> (+2.00%) ⬆️

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 97891ac...0551b74. Read the comment docs.

@iche033
Copy link
Contributor Author

iche033 commented Jun 18, 2021 •
edited
Loading

hmm the INTEGRATION_depth_camera test failed twice on homebrew. It's failing on checks that are related to #332 which should've been fixed in the changes ported from ign-rendering3 (hence the test were re-enabled). Interesting that the test is not failing in PRs targeting the ign-rendering3 and ign-rendering5 branches which also have this test enabled.

I'll probably just disable it in ign-rendering4

@chapulina
Copy link
Contributor

I'll probably just disable it in ign-rendering4

Homebrew is green on the latest commit, and I think you haven't disabled the test yet, right? Is it flaky?

@iche033
Copy link
Contributor Author

iche033 commented Jun 18, 2021

I haven't disabled the test. I just hit retry on the build. So I think the test is flaky. So far, test passed 1 out 3 times in this PR.

@chapulina
Copy link
Contributor

Got it, I'll leave it up to you if you want to leave it flaky for a while or disable it. I'd like to get this PR in soon so we can make a new 4.X release to unblock downstream PRs.

@iche033
Copy link
Contributor Author

iche033 commented Jun 18, 2021

added ifdef for apple around the checks that fail instead of disabling the whole test 0551b74

@iche033
Copy link
Contributor Author

iche033 commented Jun 18, 2021

all builds green. Merging.

@iche033 iche033 merged commit f208f9c into ign-rendering4 Jun 18, 2021
@iche033 iche033 deleted the merge_3_4_061721 branch June 18, 2021 23:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
🔮 dome Ignition Dome
Projects
None yet
Milestone
No milestone
Development

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