Added basePath as option for Model.fromGltf

[moneimne]

Fix for #5320

[pjcozzi]

@moneimne can you please

• Update CHANGES.md
• Add unit tests for all new cases, ModelSpec.js

[moneimne]

Updated

[hpinkos]

If these are both URLs, we should use the joinUrls function to concat them

[lilleyse]

It is ok this way since the cache key doesn't need to be a url, it just needs to be a unique way to identify a model.

[hpinkos]

Change this and below to } else {

Our code style convention for if/else blocks is

if (...) {
    ...
} else if (...) {
    ...
} else {
    ...
}

[hpinkos]

Set defined(options.basePath) to a variable so you don't call defined twice (lines 1191 and 1195)

[hpinkos]

These if blocks can be re-written to be one line

[hpinkos]

Those are all of the comments that I have. Functionality is great

[moneimne]

Thanks for the reviews, @hpinkos! Made the changes.

[hpinkos]

This can still be simplified a bit further

options.cacheKey = cacheKey;
if (defined(options.basePath)) {
    options.cacheKey += options.basePath;
} else {
    options.basePath = getBaseUri(url, true);
}

[moneimne]

I don't think this would do what we want. We only want to append basePath to cacheKey if a cache key isn't specified in options by the user.

[lilleyse]

Maybe the code could be simplified like:

        var cacheKey = defaultValue(options.cacheKey, getAbsoluteUri(url));
        var basePath = defaultValue(options.basePath, getBaseUri(url, true));

        if (!defined(options.basePath) && !defined(options.cacheKey)) {
            cacheKey += basePath;
        }
        
        options.cacheKey = cacheKey;
        options.basePath = basePath;

[hpinkos]

Oh, gotcha. Sorry I missed that

[moneimne]

This works! Just had to change the condition to use defined(options.basePath) without the not.

[lilleyse]

Reword this description to 'fromGltf takes a base path'

[lilleyse]

Maybe the code could be simplified like:

        var cacheKey = defaultValue(options.cacheKey, getAbsoluteUri(url));
        var basePath = defaultValue(options.basePath, getBaseUri(url, true));

        if (!defined(options.basePath) && !defined(options.cacheKey)) {
            cacheKey += basePath;
        }
        
        options.cacheKey = cacheKey;
        options.basePath = basePath;

[lilleyse]

Remove the ='' since the default basePath is derived from the url.

[moneimne]

Updated

[hpinkos]

Great, thanks @moneimne!