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

Fix Windows regresion on OgreLightVisual #228

Merged
merged 6 commits into from
Feb 4, 2021
Merged

Fix Windows regresion on OgreLightVisual #228

merged 6 commits into from
Feb 4, 2021

Conversation

ahcorde
Copy link
Contributor

@ahcorde ahcorde commented Feb 3, 2021

Related with this regression issue #226

Signed-off-by: ahcorde ahcorde@gmail.com

@ahcorde
Fix Windows regresion on OgreLightVisual
83469e1 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde ahcorde added tests Broken or missing tests / testing infra Windows Windows support labels Feb 3, 2021
@ahcorde ahcorde requested a review from iche033 February 3, 2021 20:00
@ahcorde ahcorde self-assigned this Feb 3, 2021
@github-actions github-actions bot added the 🏢 edifice Ignition Edifice label Feb 3, 2021
@osrf-triage osrf-triage added this to Inbox in Core development Feb 3, 2021
@ahcorde ahcorde mentioned this pull request Feb 3, 2021
Closed
@codecov
Copy link

codecov bot commented Feb 3, 2021 •
edited

Codecov Report

Merging #228 (38db259) into main (f9b58e6) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #228      +/-   ##
==========================================
- Coverage   56.99%   56.98%   -0.02%     
==========================================
  Files         151      151              
  Lines       14988    14989       +1     
==========================================
- Hits         8542     8541       -1     
- Misses       6446     6448       +2
Impacted Files Coverage Δ
include/ignition/rendering/base/BaseLightVisual.hh 30.09% <0.00%> (ø)
ogre/src/OgreLightVisual.cc 0.00% <0.00%> (ø)
...e/ignition/rendering/base/BaseGaussianNoisePass.hh 96.66% <0.00%> (-3.34%) ⬇️

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 f9b58e6...38db259. Read the comment docs.

@ahcorde
One more fix
1e46bf2 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fix windows warning
c6d6d38 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@chapulina chapulina moved this from Inbox to In review in Core development Feb 3, 2021
@chapulina
Copy link
Contributor

Weird to see the heightmap warning on Windows, it had been addressed on #180. I think the pragmas can be extended to wrap the whole file.

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 3, 2021

@chapulina should I fix this here ?

@chapulina
Copy link
Contributor

@chapulina should I fix this here ?

That would be great, thanks!

@ahcorde
Fix windows warning
70d83c8 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 3, 2021

@j-rivero or @chapulina any idea about this one:

D:\Jenkins\workspace\ign_rendering-pr-win\ws\ign-rendering\ogre2\src\Ogre2Light.cc(284,1): fatal error C1001: Internal compiler error. [D:\Jenkins\workspace\ign_rendering-pr-win\ws\build\ignition-rendering5\ogre2\src\ignition-rendering5-ogre2.vcxproj]

@chapulina
Copy link
Contributor

any idea about this one:

That looks very strange, I retriggered the build 🤔

@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 3, 2021

@osrf-jenkins retest this please

iche033
iche033 reviewed Feb 3, 2021
View reviewed changes
ogre/src/OgreLightVisual.cc
this->dataPtr->line->setMaterial("Default/TransGreen");
MaterialPtr lightVisualMaterial =
this->Scene()->Material("Default/TransGreen")->Clone();
this->SetMaterial(lightVisualMaterial, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed? since we're not modifying the material, we don't need a copy right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed e8ea4e7

@iche033 iche033 mentioned this pull request Feb 3, 2021
Merged
@ahcorde
Use the pointer material instead of cloning
e8ea4e7 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Fixed MacOS warnings
38db259 
Signed-off-by: ahcorde <ahcorde@gmail.com>
@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 4, 2021

@osrf-jenkins retest this please

1 similar comment
@ahcorde
Copy link
Contributor Author

ahcorde commented Feb 4, 2021

@osrf-jenkins retest this please

@ahcorde ahcorde mentioned this pull request Feb 4, 2021
Closed
chapulina
chapulina approved these changes Feb 4, 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.

CI is happy and I believe all comments have been addressed. Merging to fix CI.

@chapulina chapulina merged commit 1f53897 into main Feb 4, 2021
Core development automation moved this from In review to Done Feb 4, 2021
@chapulina chapulina deleted the ahcorde/fix/regresion branch February 4, 2021 18:17
@j-rivero j-rivero removed this from Done in Core development May 6, 2022
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

@iche033 iche033 Awaiting requested review from iche033 iche033 is a code owner

Assignees

@ahcorde ahcorde

Labels
🏢 edifice Ignition Edifice tests Broken or missing tests / testing infra Windows Windows support
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ahcorde @chapulina @iche033