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 an avatar crash. #1326

Merged
merged 3 commits into from
Sep 7, 2021
Merged

Fix an avatar crash. #1326

merged 3 commits into from
Sep 7, 2021

Conversation

ctrlaltdavid
Copy link
Collaborator

@ctrlaltdavid ctrlaltdavid commented Aug 29, 2021 •
edited
Loading

Some avatar geometries trigger this crash:

Exception thrown: read access violation.
**radius** was nullptr.

>	interface.exe!MultiSphereShape::calculateSphereLines(std::vector<std::pair<glm::vec<3,float,0>,glm::vec<3,float,0>>,std::allocator<std::pair<glm::vec<3,float,0>,glm::vec<3,float,0>>>> & outLines, const glm::vec<3,float,0> & center, const float & radius, const int & subdivisions, const glm::vec<3,float,0> & direction, const float & percentage, std::vector<glm::vec<3,float,0>,std::allocator<glm::vec<3,float,0>>> * edge) Line 593	C++
 	interface.exe!MultiSphereShape::calculateChamferBox(std::vector<std::pair<glm::vec<3,float,0>,glm::vec<3,float,0>>,std::allocator<std::pair<glm::vec<3,float,0>,glm::vec<3,float,0>>>> & outLines, const std::vector<float,std::allocator<float>> & radiuses, const std::vector<glm::vec<3,float,0>,std::allocator<glm::vec<3,float,0>>> & axes, const glm::vec<3,float,0> & translation) Line 515	C++
 	interface.exe!MultiSphereShape::calculateDebugLines() Line 493	C++
 	interface.exe!Avatar::computeMultiSphereShapes() Line 1614	C++
 	interface.exe!Avatar::rigReady() Line 1580	C++
 	[External Code]	
 	interface.exe!Model::updateGeometry() Line 313	C++
 	interface.exe!CauterizedModel::updateGeometry() Line 32	C++
 	interface.exe!Model::simulate(float deltaTime, bool fullUpdate) Line 1450	C++
 	interface.exe!SkeletonModel::simulate(float deltaTime, bool fullUpdate) Line 160	C++
 	interface.exe!MyAvatar::simulate(float deltaTime, bool inView) Line 952	C++
 	interface.exe!MyAvatar::update(float deltaTime) Line 800	C++
 	interface.exe!AvatarManager::updateMyAvatar(float deltaTime) Line 167	C++
 	interface.exe!Application::update(float deltaTime) Line 6730	C++
 	interface.exe!Application::idle() Line 5375	C++
 	interface.exe!Application::event(QEvent * event) Line 4323	C++
 	[External Code]	
 	interface.exe!Application::notify(QObject * object, QEvent * event) Line 4280	C++
 	[External Code]	
 	interface.exe!main(int argc, const char * * argv) Line 448	C++
 	interface.exe!WinMain(HINSTANCE__ * __formal, HINSTANCE__ * __formal, char * __formal, int __formal) Line 97	C++
 	[External Code]

Aitolda encounteered this recently. Appears to be the same crash as in the following issues:

QA should include checking avatar debug rendering.

@digisomni digisomni added this to In progress in 2022.1.0 Selene Release via automation Aug 29, 2021
@digisomni digisomni added this to the 2021.2.0 Selene Release milestone Aug 29, 2021
@vircadia-build-notifier
Copy link

The following links are available:
build (macOS-latest, client)

build (macOS-latest, full)

build (ubuntu-18.04, full)

build (windows-latest, full)

This was referenced Aug 30, 2021
Open
Closed
@Aitolda
Copy link
Collaborator

Aitolda commented Aug 30, 2021

Works exactly as intended. Avatar no longer causes insta-crash.

@Aitolda Aitolda added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Aug 30, 2021
HifiExperiments
HifiExperiments reviewed Sep 1, 2021
View reviewed changes
libraries/physics/src/MultiSphereShape.cpp Outdated
}
}
if (radiuses.size() == 0) {
radiuses.push_back(0.0f);
Copy link
Collaborator

Choose a reason for hiding this comment

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

interesting…is there a way we could instead detect lower down in the callstack (in calculateChamferBox, or lower) that radiuses (radii??) is empty? I worry someone else will call it and crash

Copy link
Collaborator Author

@ctrlaltdavid ctrlaltdavid Sep 4, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

There's no obvious fix that I can see. To handle an empty radiuses further down the call stack would be a matter of working through the code to see how to avoid calculating and displaying a sphere, and handle possible assumptions that there is in fact a sphere to display.

I could either add a "FIXME" comment to the proposed change, or pursue things further down the stack sometime "soon".

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we just early exit out of calculateChamferBox at the top if radiuses.size() == 0? does that crash somewhere else in the code?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That alternative change also fixes the crash, though I don't know what side-effects not doing the chamfer box calculations may have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

can we ever really know the side-effects of doing or not doing things

@ctrlaltdavid ctrlaltdavid added needs testing (QA) The PR is ready for testing and removed QA Approved The PR has been tested successfully. labels Sep 6, 2021
@vircadia-build-notifier
Copy link

The following links are available:
build (macOS-latest, client)

build (macOS-latest, full)

build (ubuntu-18.04, full)

build (windows-latest, full)

@vircadia-build-notifier
Copy link

The following links are available:
build (macOS-latest, client)

build (macOS-latest, full)

build (ubuntu-18.04, full)

build (windows-latest, full)

@Aitolda
Copy link
Collaborator

Aitolda commented Sep 7, 2021

Update seems to be working.

2022.1.0 Selene Release automation moved this from In progress to Reviewer approved Sep 7, 2021
@digisomni digisomni added CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing needs CR (code review) labels Sep 7, 2021
@digisomni digisomni merged commit 542e070 into vircadia:master Sep 7, 2021
2022.1.0 Selene Release automation moved this from Reviewer approved to Done Sep 7, 2021
@digisomni digisomni removed this from Done in 2022.1.0 Selene Release Sep 7, 2021
@digisomni digisomni added this to In progress in 2021.1.3 Eos Release via automation Sep 7, 2021
@digisomni digisomni changed the title Fix avatar crash Fix an avatar crash. Sep 7, 2021
@ctrlaltdavid ctrlaltdavid deleted the fix/avatar-crash branch September 7, 2021 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@HifiExperiments HifiExperiments HifiExperiments approved these changes

Assignees
No one assigned
Labels
bugfix CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully.
Projects
No open projects
2021.1.3 Eos Release
In progress
Milestone
2021.1.3 Eos Release
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@ctrlaltdavid @vircadia-build-notifier @Aitolda @HifiExperiments @digisomni