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.

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

Stream Texture Import Bug from official examples #462

Closed
ManSoorSour opened this issue Jul 2, 2019 · 13 comments
Closed

Stream Texture Import Bug from official examples #462

ManSoorSour opened this issue Jul 2, 2019 · 13 comments

Comments

@ManSoorSour
Copy link

ManSoorSour commented Jul 2, 2019

Im currently trying to test out the stream option but it seems like it doesn't import the textures correctly. If I run the official example scene "StreamTestScene", I get the following result:

Test

The URI option works great, only the stream option gives me red/white question mark textures. The same bug appears for every model imported through the stream option.

@ManSoorSour
Copy link
Author

ManSoorSour commented Jul 3, 2019

GLB/GLTF-Binary-Models imported through the stream option works great. I actually don't get it, the only difference between the Stream & URI option is the ExternalDataLoader property from ImportOptions. But both looks fine for me... so why does the above bug only appear in stream option?

@Dryra
Copy link

Dryra commented Jul 11, 2019

Hello,
I'm having the same issue, is there any news on this ?

@PWendtInneo
Copy link

Hi there, I just wanted to know, if this is still an issue. I had this Issue too, when it first came up, and I'm waiting for this to get solved before I'll update the plugin in my project.

@Stephengower
Copy link

To workaround this issue, in GLTFSceneImporter.cs, in the ConstructImageBuffer method, change the LoadStream call to use LoadStreamSync instead.

//  await _options.ExternalDataLoader.LoadStream(image.Uri);
 _options.ExternalDataLoader.LoadStreamSync(image.Uri);

For some reason, the async LoadStream method only works once, so only the first texture is loaded, and all others are empty.

@Dryra
Copy link

Dryra commented Sep 13, 2019

@Stephengower Thank you for the info! Saved me a lot of time! it works now as expected

@Stephengower
Copy link

Yeah, the current release is just flat out broken due to this, and it looks like it's been this way for a couple months at least. Maybe all the devs took the summer off.

@Dryra
Copy link

Dryra commented Sep 16, 2019

I take the works as expected back, it's causing memory leaks on iOS.

@yaqinh
Copy link

yaqinh commented Sep 19, 2019

vr_table.zip
Hello, I still see this issue. The texture is wrong for some files I have tried. (See the attachment GLTF file). @Stephengower 's method resolves this bug but is it the official way to do it? Hope the dev team could fix this ASAP. Thanks!

@Dryra
Copy link

Dryra commented Sep 20, 2019

@yaqinh No it is not, and it does not really fix the issue completely as it causes memory leaks on iOS. I guess we have to wait until the devs are back.

@yaqinh
Copy link

yaqinh commented Sep 20, 2019

@Dryra , yeah, my platform is UWP and I have not found any issue yet. But, yeah memory leak on IOS is important

@fagerburg
Copy link
Contributor

fagerburg commented Nov 14, 2019

From what I can see, the issue stems from attempting to use File.OpenRead asynchronously when it wasn't created to natively work that way, at least initially. I don't have an iOS device to test this issue on so I can't reproduce the memory leak but by modifying the LoadFileStream method in FileLoader.cs to look like this below, the textures now load correctly.

private Task LoadFileStream(string rootPath, string fileToLoad)
{
	string pathToLoad = Path.Combine(rootPath, fileToLoad);
	if (!File.Exists(pathToLoad))
	{
		throw new FileNotFoundException("Buffer file not found", fileToLoad);
	}

	Task fileOpenTask = Task.Run(() => { LoadedStream = File.OpenRead(pathToLoad); });
	fileOpenTask.Wait();
	return fileOpenTask;

}

If someone can confirm whether this fixes the issue, I'll go ahead and make a pull request and see if i can get it merged in.

@fagerburg
Copy link
Contributor

@AdamMitchell-ms Are you ok with the above implementation to resolve this issue? This also would resolve issue #484

@fagerburg
Copy link
Contributor

this has been resolved by @mhochk in #532 and can be closed

@mhochk mhochk closed this as completed Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants