-
Notifications
You must be signed in to change notification settings - Fork 45
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
Out of band assets #285
Out of band assets #285
Conversation
|
||
val source = assetData?.getMap("source") ?: return false // Do not handle the asset. | ||
|
||
CoroutineScope(Dispatchers.IO).launch { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ErikUggeldahl, your input here would be great on the best way to implement coroutine scope management (canceling it if the view is destroyed).
|
||
// Handle release mode (URL instead of asset id) | ||
// Resource needs to be loaded in release mode | ||
// https://github.com/facebook/react-native/issues/24963#issuecomment-532168307 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lancesnider would be great if we can give extra testing on this. It's handled differently between a dev and release build.
@@ -104,6 +108,10 @@ type Props = { | |||
testID?: string; | |||
alignment?: Alignment; | |||
artboardName?: string; | |||
/** | |||
* @experimental This is an experimental feature and may change without a major version update (breaking change). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opinions on marking this as experimental?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably a good idea, especially since we're likely to have a major overhaul of the runtime relatively soon.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great!
I really like the assetsHandled
object. I think it'll be easier for users to grasp than assetLoader
which requires some extra steps. If I'm understanding correctly the downside is that you can only define a referenced asset at load time, but not at runtime. I'm assuming that has to do with the limitation you mentioned in the video where we can't have a callback?
I love that you don't need to add assets to Bundled Assets
in iOS, but I'm not sure I understand why.
// path: 'fonts', // only needed for Android assets | ||
// }, | ||
}, | ||
'referenced-image-2929282': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I'd prefer not to need the ID, I think using it by default is probably necessary, given the fact that it's possible to have multiple assets with the same name. I could see users being very confused when both fonts named Tomorrow
display the same even though one of them is actually supposed to be a different weight.
I really like the idea of being able to override the default behavior with includeId
. Do you have thoughts on that being a global attribute, rather than on a per-asset basis?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the same fonts (or asset) we will use the same ID! You also have the option to rename these to anything you'd like.
Expanding this APIs ability is something we can also add in the future, if needed.
example/src/OutOfBandAssets.tsx
Outdated
// | ||
// The key of the map is the unique asset identifier (as exported in the Editor), | ||
// which is a combination of the asset name and its unique identifier. | ||
assetsHandled={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's your thinking on the name assetsHandled
? My first thought would be to call this referencedAssets
to align it with the naming in the editor, but maybe that's too restricting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also been toying with thinking of a different name for this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like referencedAssets
as a name!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
@@ -104,6 +108,10 @@ type Props = { | |||
testID?: string; | |||
alignment?: Alignment; | |||
artboardName?: string; | |||
/** | |||
* @experimental This is an experimental feature and may change without a major version update (breaking change). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's probably a good idea, especially since we're likely to have a major overhaul of the runtime relatively soon.
sourceAsset?: string; | ||
sourceAssetId?: string; | ||
path?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it worth being explicit that exactly one of sourceUrl
, sourceAsset
and path
is required?
type FileAssetSource =
| {
sourceUrl: string
sourceAsset?: never
path?: never
sourceAssetId: string
}
| {
sourceAsset: string
sourceUrl?: never
path?: never
sourceAssetId: string
}
| {
path: string
sourceUrl?: never
sourceAsset?: never
sourceAssetId: string
}
```
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean as a doc comment? Or do you have a suggestion code wise?
This as TypeScript should ensure that only one of sourceUrl, sourceAsset, or path is provided at a time
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current code, it would still be valid if sourceUrl
, sourceAsset
, path
, and sourceAssetId
were all undefined. It would also be valid if all 4 were defined.
If you want the type to expect exactly 1 of the 4 values, you might write it like this:
export type FileAssetSource =
| { sourceUrl: string; sourceAsset?: never; sourceAssetId?: never; path?: never }
| { sourceAsset: string; sourceUrl?: never; sourceAssetId?: never; path?: never }
| { sourceAssetId: string; sourceUrl?: never; sourceAsset?: never; path?: never }
| { path: string; sourceUrl?: never; sourceAsset?: never; sourceAssetId?: never };
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh sorry @lancesnider, I thought you were quoting code above that was already there 🤦 . Yes, this makes sense to add
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@lancesnider I played around with this, and this might have other unintended side effects.
The source_
props are not intended to be set directly by the user. I should have actually named the path
above sourcePath
. They will be mapped from the user supplied value.
So if a user sets any of the source_
options it will fail to work. For simplicity let's keep it as is, and make this a documentation concern.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, this was definitely a nitpick. Let's not worry about it. 👍
// path: 'images', // only needed for Android assets | ||
// }, | ||
}, | ||
'referenced_audio-2929340': { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no scenario where the asset IDs might change, is there? For example, if the Rive file gets copied or if you reimport a PSD? That could be painful if you have a giant object with ID's included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's why the approach for the other runtimes are a callback mechanism where you can read the name + id and query the file. That's not possible for React Native at this stage, which is why this API is different.
The best way to circumvent this is to provide a "name" only fallback and forgo the id
Adds clarity and formatting.
Couple more quick formatting changes.
Great work and thank you so much for doing this ❤️ I'm not sure what is the "ID" tbh, how would we obtain it so we know what to set the key to, but if it it depends on the Thank you |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There may be some more RN-specific comments left, but generally from the Swift side of things, things look great! I left some comments about things we previously discussed during a live walkthrough.
ios/RNRiveError.swift
Outdated
@@ -64,6 +64,10 @@ func createMalformedFileError() -> NSError { | |||
return NSError(domain: RiveErrorDomain, code: RiveErrorCode.malformedFile.rawValue, userInfo: [NSLocalizedDescriptionKey: "Malformed Rive File", "name": "Malformed"]) | |||
} | |||
|
|||
func createAssetFileError(_ assetName: String) -> NSError { | |||
return NSError(domain: RiveErrorDomain, code: RiveErrorCode.malformedFile.rawValue, userInfo: [NSLocalizedDescriptionKey: "Could not load Rive asset: \(assetName)", "name": "Malformed"]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpicky, but I would maybe use .fileNotFound
rather than malformed as the error code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
example/src/OutOfBandAssets.tsx
Outdated
// | ||
// The key of the map is the unique asset identifier (as exported in the Editor), | ||
// which is a combination of the asset name and its unique identifier. | ||
assetsHandled={{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've also been toying with thinking of a different name for this.
} | ||
} | ||
|
||
private func processAssetBytes(_ data: Data, asset: RiveFileAsset, factory: RiveFactory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth having another check here to validate that data.isEmpty == false
.
} | ||
|
||
private func splitFileNameAndExtension(fileName: String) -> (name: String?, ext: String?)? { | ||
let components = fileName.split(separator: ".") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any documentation surrounding naming? This might not work as intended, for example, if a user has an asset name with a .
in it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. See commit: 554fd77
Using Swift provided methods now
return (name: String(components[0]), ext: String(components[1])) | ||
} | ||
|
||
private func loadResourceAsset(sourceAsset: String, path: String?, listener: @escaping (Data) -> Void) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Discussed live, but path is currently ignored. This is understandable as a first-pass, with the expectation being that the file is already in the bundle's Resources
directory. There may be a time where we wish to expand this API to support further paths.
Love the ability to load assets different ways. As mentioned by others, we still really need the ability to replace an asset at run time. For our use case, we have an image that represents a topic. The user can shuffle this topic, so the current image gets replaced with another image. These images are dynamic (UGC). So we only know them at run time. In our case we'd want have two referenced images that we toggle between, allowing a the theoretically infinite number or images to be displayed as there is no hard limit to the number of shuffles. If that was too long to follow: we have a need for the API to also allow us to replace an image after the animation loads any number of times, like the JS web rumtime supports (though I do think this API is simpler/cleaner so if there's a way to combine the ideas that would be awesome). |
Thanks for doing this! This is definitely something we can take a lot of advantage from and reduces some of the limitations we are currently experiencing. |
@HayesGordon I'm wondering if I misunderstood something. @lancesnider said "the downside is that you can only define a referenced asset at load time, but not at runtime," so I assumed that was the case. In the video you talked about "hot reloading" so I thought you meant during development, but looking at the code, It looks to me like we can hotswap the assets in production builds. For example, if the references/urls in the assetsHandled map change, they'll be updated in the Rive view without reloading the animation. Am I correct? Other thoughts after watching the video 4 times:
|
Sorry, that was poor phrasing. I meant that this lets you load a different font at runtime, but only when the Rive element first loads for the user. You couldn't keep a reference to the asset and swap it after the Rive has loaded. |
This looks really promising and I'm excited to use it! This will unblock some Android development efforts for us. |
@lancesnider What you want can be accomplished with this API (and, if I understand the code correctly, it is already implemented in this PR). The term I would use here is "hot swapping": being able to change the font, image, sound, etc. for an animation without reloading or otherwise interrupting the animation state. In the web API there is a reference, but in this API you would just update the value of the prop.Props values at initial render:
Change the value of Inter-594377 to hot swap the image for a new one without disrupting or reinitializing the animation, preserving its state:
|
This is an awesome feature! I will definitely be using this because my company asked me to do a POC to move all our RN graphics to Rive. The major issue was not being able to remotely load headshots for our items into the template. I'm hoping this gets merged and released soon. |
The ID comes from the editor when you export the asset! And yes, this will still allow you to reuse assets. But maybe we need to allow you to optionally pass the ID, else it will only use the name of the asset. |
sourceAsset?: string; | ||
sourceAssetId?: string; | ||
path?: string; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the current code, it would still be valid if sourceUrl
, sourceAsset
, path
, and sourceAssetId
were all undefined. It would also be valid if all 4 were defined.
If you want the type to expect exactly 1 of the 4 values, you might write it like this:
export type FileAssetSource =
| { sourceUrl: string; sourceAsset?: never; sourceAssetId?: never; path?: never }
| { sourceAsset: string; sourceUrl?: never; sourceAssetId?: never; path?: never }
| { sourceAssetId: string; sourceUrl?: never; sourceAsset?: never; path?: never }
| { path: string; sourceUrl?: never; sourceAsset?: never; sourceAssetId?: never };
See commit: 5138fa4 We will now prefer name+id, and if not found, fallback to only the name. |
@tslater we will add this as a separate PR |
@HayesGordon super pumped about the release! To confirm, hot swapping assets after initial animation load is possible with this API, but not implemented in this PR/release (8.4.0) but will be in a future PR/release by just the values in the assetsHandled prop? |
Basically, yes! I'm thinking on the riveRef.current?.setAsset("Inter-594377", require('./assets/Inter-594377.ttf'); |
@HayesGordon I just tried to load oob assets using Them I tried copying the I re-exported in the editor and the filename of the referenced image exported was I also tried the font/audio files (but with a lot less effort) and those seem to not be loading either. Not sure if I'm missing a step? |
@tslater are you including the out of band assets: referencedAssets={{
'Inter-594377': {
source: require('./assets/Inter-594377.ttf'),
// source: {
// fileName: 'Feather.ttf',
// path: 'fonts', // only needed for Android assets
// },
},
'referenced-image-2929282': {
source: require('./assets/referenced-image-2929282.png'),
// source: {
// uri: 'https://picsum.photos/id/270/500/500',
// },
// source: {
// fileName: 'referenced-image-2929282.png',
// path: 'images', // only needed for Android assets
// },
},
'referenced_audio-2929340': {
source: require('./assets/referenced_audio-2929340.wav'),
// source: {
// fileName: 'referenced_audio-2929340.wav',
// path: 'audio', // only needed for Android assets
// },
},
}} I just tried with the packaged version 8.4.0 and it's working my side. Are you seeing any errors if you subscribe to onError: onError={(riveError: RNRiveError) => {
console.log(riveError);
}} If you're still encountering issues, can you make a sample project and share that so we can investigate. |
I tried both requiring them (and including them) and remote uris. I am subscribed to errors but no errors. I'll try a fresh project and let you know. |
For reference, I am using an expo dev client on iOS. Here's a sample project: |
Tagging @lancesnider as he mentioned testing things on Expo, maybe he has some insight as well. |
I am running into this as well, there is no error, but the image is not loading. I am using expo with dev builds. @HayesGordon I can create a reproducible example if needed. Let me know and thanks for the work on this! |
@tslater Good news! I just got your example file working. Here are the 3 things I did: First I had to prebuild the Expo app to get the native libraries working. I'm assuming you did this as well if you were able to get the .riv working without the referenced assets. If not, here are the steps:
Next I needed to make sure the assets and the .riv were being copied into the bundle.
At this point, the .riv was displaying, but the referenced assets weren't. It looks like it was trying to load the wrong .riv. When setting the
With these changes I was able to run |
@lancesnider Thanks for checking this out so quickly! For examoke, does this work?
|
@tslater I was able to get the remote URI loading working following his steps. What I am still struggling with is if the Rive file is not a local asset in the bundle. If I load the Rive file from a url such as |
@john-sportshub That is a good catch. So it seems lik OOB assets currently does not work for rive animations that are loaded via URL. |
@tslater and @john-sportshub, I'll push a fix for getting OOB assets to work when using a URL. Edit: this requires a change in the underlying iOS runtime first |
@HayesGordon Bummer about the needed changes to the iOS runtime, but bug thanks for being so on top of this. |
@tslater it should be a small change, but it depends on when we will be able to do a new iOS release. There are other conflicting work that might block a release. I'm hoping we can get it out this week |
@HayesGordon Is there a separate issue for it, or are we tracking here? |
This PR adds out of band asset support to Rive React Native.
rev file used in the video:
out_of_band.rev.zip
Video:
https://drive.google.com/file/d/1jDZY2MRLhLfV8EOpRz3rVcCRbTdkv6sI/view?usp=sharing
Edit: