Wait for textures to be loaded before adding model to the scene #597

NolaDonato · 2016-06-02T21:47:01Z

Added IAssetEvents interface to listen for asset load events
onTextureLoaded: raised when texture successfully loaded
onTextureError: raised when texture fails to load
onModelLoaded: raised when scene graphi successfully loaded
onModelError: raised when model fails to load
onAssetLoaded: raised when model and all textures are loaded
Modify GVRContext to use GVRImporter.loadModel
GVRImporter.loadModel takes a GVRScene as an argument. If it is present, the model will be added to the scene after all the textures have been loaded

GearVRf-DCO-1.0-Signed-off-by: Nola Donato nola.donato@samsung.com

onTextureLoaded: raised when texture successfully loaded onTextureError: raised when texture fails to load onModelLoaded: raised when scene graphi successfully loaded onModelError: raised when model fails to load onAssetLoaded: raised when model and all textures are loaded

rahul27 · 2016-06-03T00:42:08Z

GVRf/Framework/framework/src/main/java/org/gearvrf/GVRContext.java

+     * 
+     * @return The asset loader associated with this context.
+     */
+    public GVRImporter getAssetLoader() {


The function description says that the call would return an asset loader but it returns an importer. We would decide on a name either rename the call to getImporter or rename GVRImporter to AssetLoader.

I will rename it GVRAssetLoader then. I think that is a better name. It was a private class until now so nobody was using it externally.

From: rahul [mailto:notifications@github.com]
Sent: Thursday, June 2, 2016 5:42 PM
To: Samsung/GearVRf GearVRf@noreply.github.com
Cc: Nola Donato nola.donato@samsung.com; Author author@noreply.github.com
Subject: Re: [Samsung/GearVRf] Wait for textures to be loaded before adding model to the scene (#597)

In GVRf/Framework/framework/src/main/java/org/gearvrf/GVRContext.javahttps://github.com//pull/597#discussion_r65642518:

@@ -193,6 +194,19 @@ public GVRActivity getActivity() {

} /**

* Get the {@link GVRImporter} to use for loading assets.

*

* A {@code GVRImporter} loads models asynchronously from your applications's

* local storage or the network.

*

*

* @return The asset loader associated with this context.

*/

public GVRImporter getAssetLoader() {

The function description says that the call would return an asset loader but it returns an importer. We would decide on a name either rename the call to getImporter or rename GVRImporter to AssetLoader.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/597/files/fb58b0721681e130ce48eec2b79e1fc44b7ecf78#r65642518, or mute the threadhttps://github.com/notifications/unsubscribe/AQw1S8-MZszN4PJdSdbOzfcpbZJrQigpks5qH3hjgaJpZM4ItA3y.

Also, there seems to be two conflicting ways of getting an importer object - using this call getAssetLoader or creating one using GVRImporter importer = new GVRImporter(context). This might be a little too confusing. We need to decide on one or the other.

Yes, we do. The preferred way will be to use the asset loader. For ALL assets, even textures. I took a deep look into the texture loading issues because we have some bugs there. I have a plan to fix it by making the whole texture loading thing a lot simpler but that will take a while.

So I am doing it in several stages. The first stage is handling the model loading. If we use the new API which waits for all the textures to be loaded before adding the model to the scene, it will fix some of the problems.

The second stage will be to move texture loading from GVRContext to the asset loader. This is a lot harder because I don’t want to just propagate the existing API. I would like to make something cleaner that will immediately return a GVRTexture (not a future texture or any callback thing). This texture will “magically” load behind the scenes without the user having to do anything. When all the textures for a scene object actually have data present, the lower level layer will render it. Of course, this requires changes to the C++ layer (lots of them) and changes to the implementation of the Java API. I have started this work and will release that in stages too. It is in the “texturereform” branch of my personal repo if you want to look at it.

The final stage will be to change all the samples to use the newer Java API and completely deprecate the old one.

Be patient and it will get less confusing…

From: rahul [mailto:notifications@github.com]
Sent: Thursday, June 2, 2016 5:51 PM
To: Samsung/GearVRf GearVRf@noreply.github.com
Cc: Nola Donato nola.donato@samsung.com; Author author@noreply.github.com
Subject: Re: [Samsung/GearVRf] Wait for textures to be loaded before adding model to the scene (#597)

In GVRf/Framework/framework/src/main/java/org/gearvrf/GVRContext.javahttps://github.com//pull/597#discussion_r65643197:

@@ -193,6 +194,19 @@ public GVRActivity getActivity() {

} /**

* Get the {@link GVRImporter} to use for loading assets.

*

* A {@code GVRImporter} loads models asynchronously from your applications's

* local storage or the network.

*

*

* @return The asset loader associated with this context.

*/

public GVRImporter getAssetLoader() {

Also, there seems to be two conflicting ways of getting an importer object - using this call getAssetLoader or creating one using GVRImporter importer = new GVRImporter(context). This might be a little too confusing. We need to decide on one or the other.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHubhttps://github.com//pull/597/files/fb58b0721681e130ce48eec2b79e1fc44b7ecf78#r65643197, or mute the threadhttps://github.com/notifications/unsubscribe/AQw1S55ZvuCrFezBg4ckJ8UoBo67ydRzks5qH3qBgaJpZM4ItA3y.

Clarified documentation, removed uneccessary functions, naming

liaxim · 2016-06-06T16:22:12Z

GVRf/Framework/framework/src/main/java/org/gearvrf/GVRAssetLoader.java

+     * Listens for texture load events and raises the "onAssetLoaded"
+     * event after all textures have been loaded.
+     */
+    static public class AssetEventListener implements IAssetEvents


Is AssetEventListener supposed to be used by end users? If not, how about making it non public?

Yes it does not need to be public

liaxim · 2016-06-06T17:33:55Z

GVRf/Framework/framework/src/main/java/org/gearvrf/GVRAssetLoader.java

+     * @throws IOException 
+     *
+     */
+    public GVRModelSceneObject loadModel(String filePath, GVRResourceVolume.VolumeType volumeType,


This is kind of cosmetic but I'd say the final API will be cleaner if we just expose loadModelFromUrl, loadModelFromAssets, loadModelFromSdcard, etc instead of passing the volumeType. It would make for easier to read code and possibly make it a bit less error-prone. Also we might request URL instance from the user in the case of loadModelFromUrl so we know we get valid input, etc.

For example the existing API would return a null model instance in case of an invalid URL. The user needs to know what to look for in logcat to find out that.

I think it would be even better if we got rid of the volume type altogether and just put a prefix on the filename like HTTP does, maybe "SD:" for the SD card and "ASSETS:" for the assets directory? I just preserved the arguments of the existing API but moved the functions to GVRAssetLoader because I didn't want to break existing applications.

I wouldn't mind at all removing the volume type or at least hiding it from public view. Prefixing as suggested could work.

liaxim · 2016-06-06T17:41:33Z

The direction is good. Isn't the title of this particular pull request misleading though? Please squash the commits at some point.

NolaDonato · 2016-06-06T21:45:34Z

Squashing commits and reopening new pull request #601

Exposes GVRAssetLoader class for loading assets - replaces PR #597

NolaDonato added 4 commits May 27, 2016 12:51

Issue onAssetLoaded for models with no textures

cf1a1ef

added copyright header

3fc6ba4

GVRContext uses new GVRImporter.loadModel

fb58b07

rahul27 reviewed Jun 3, 2016
View reviewed changes

NolaDonato added 2 commits June 2, 2016 18:23

Address rahul's comments

f9e7657

Clarified documentation, removed uneccessary functions, naming

Fully qualify usage of "this"

32c0e9b

liaxim reviewed Jun 6, 2016
View reviewed changes

liaxim mentioned this pull request Jun 6, 2016

Fix gvr-jassimp timing error gearvrf/GearVRf-Demos#181

Merged

liaxim reviewed Jun 6, 2016
View reviewed changes

NolaDonato closed this Jun 6, 2016

liaxim added a commit that referenced this pull request Jun 10, 2016

Merge pull request #601 from NolaDonato/loadmodel2

92bd214

Exposes GVRAssetLoader class for loading assets - replaces PR #597

NolaDonato deleted the loadmodel branch June 14, 2016 07:39

NolaDonato mentioned this pull request Apr 11, 2018

Transform and state sorting reform #1827

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wait for textures to be loaded before adding model to the scene #597

Wait for textures to be loaded before adding model to the scene #597

NolaDonato commented Jun 2, 2016

rahul27 Jun 3, 2016

NolaDonato Jun 3, 2016

rahul27 Jun 3, 2016

NolaDonato Jun 3, 2016

liaxim Jun 6, 2016

NolaDonato Jun 6, 2016

liaxim Jun 6, 2016

NolaDonato Jun 6, 2016

liaxim Jun 9, 2016

liaxim commented Jun 6, 2016

NolaDonato commented Jun 6, 2016

Wait for textures to be loaded before adding model to the scene #597

Wait for textures to be loaded before adding model to the scene #597

Conversation

NolaDonato commented Jun 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

liaxim commented Jun 6, 2016

NolaDonato commented Jun 6, 2016