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

Update loops to prevent crash on some GLTF models. #1545

Merged
merged 3 commits into from
Dec 23, 2021
Merged

Update loops to prevent crash on some GLTF models. #1545

merged 3 commits into from
Dec 23, 2021

Conversation

ksuprynowicz
Copy link
Contributor

Fixes #1542

@ksuprynowicz ksuprynowicz mentioned this pull request Dec 20, 2021
Closed
@HifiExperiments HifiExperiments added allow-build-upload Allows the upload of a build for non white-listed users CR Approved At least one code reviewer has approved the PR. model: gltf/glb needs testing (QA) The PR is ready for testing labels Dec 20, 2021
@digisomni digisomni added the rebuild rebuild through the GithubActions label Dec 20, 2021
@digisomni digisomni changed the title Changes to for loops needed to prevent crash on some GLTF models Update loops to prevent crash on some GLTF models. Dec 20, 2021
@vircadia-build-notifier
Copy link

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

build (macOS-10.15, full)

build (ubuntu-18.04, full)

build (windows-latest, full)

build (self-hosted_debian-11_aarch64, full)

ksuprynowicz and others added 2 commits December 20, 2021 18:49
@ksuprynowicz @ctrlaltdavid
Update libraries/model-serializers/src/GLTFSerializer.cpp
7106482 
Co-authored-by: David Rowe <david@ctrlaltstudio.com>
@ksuprynowicz @ctrlaltdavid
Update libraries/model-serializers/src/GLTFSerializer.cpp
a7883fc 
Co-authored-by: David Rowe <david@ctrlaltstudio.com>
@digisomni digisomni added this to the 2022.1.1 Selene Release milestone Dec 20, 2021
@vircadia-build-notifier
Copy link

The following links are available:
build (macOS-10.15, full)

build (macOS-10.15, client)

build (ubuntu-18.04, full)

build (windows-latest, full)

build (self-hosted_debian-11_aarch64, full)

@Aitolda
Copy link
Collaborator

Aitolda commented Dec 22, 2021

Do you have any of the models that were causing the crash for testing?

@ksuprynowicz
Copy link
Contributor Author

ksuprynowicz commented Dec 22, 2021 •
edited
Loading

Do you have any of the models that were causing the crash for testing?

Here's the link to them:
https://drive.google.com/file/d/1fkCb173ts0QlMuZjtMTdJpr0rHIKqQ51/view?usp=sharing
One of them is immediately crashing current release.

@ksuprynowicz
Copy link
Contributor Author

I tested it on Linux, and it fixed the bug. This PR works fine and on the same world current master branch crashes immediately.

@Aitolda
Copy link
Collaborator

Aitolda commented Dec 23, 2021

Here's the link to them:
https://drive.google.com/file/d/1fkCb173ts0QlMuZjtMTdJpr0rHIKqQ51/view?usp=sharing
One of them is immediately crashing current release

@Aitolda
Copy link
Collaborator

Aitolda commented Dec 23, 2021

I could not reproduce the crash on the current version using Windows 10 with any of the models with either the current version (Selene) OR this PR. However, I went to various domains and had no issues with models that weren't already present with Selene, so it doesn't appear to have broken anything either. Perhaps this bug is unique to Linux?

@digisomni digisomni added QA Approved The PR has been tested successfully. and removed needs testing (QA) The PR is ready for testing labels Dec 23, 2021
@digisomni digisomni merged commit 0026aba into vircadia:master Dec 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ctrlaltdavid ctrlaltdavid ctrlaltdavid approved these changes

@HifiExperiments HifiExperiments HifiExperiments approved these changes

Assignees
No one assigned
Labels
allow-build-upload Allows the upload of a build for non white-listed users bugfix CR Approved At least one code reviewer has approved the PR. QA Approved The PR has been tested successfully. rebuild rebuild through the GithubActions
Projects
None yet
Milestone
2022.1.1 Selene Release
Development

Successfully merging this pull request may close these issues.

Crash in GLTF serializer
6 participants
@ksuprynowicz @vircadia-build-notifier @Aitolda @ctrlaltdavid @HifiExperiments @digisomni