Jump to content

Insufficient check for .manifest file version


FunFetched
 Share

Recommended Posts

I'm currently working on a project that has both a live and development version, utilizing offline storage and .manifest files for all of my Scene objects. Everything works as expected until I run the old project after having just run the new one in Chrome, specifically. The latest versions of the .scene files are always loaded from the cache, despite the older version number in their .manifest files. I did some digging, and found this:

Database.prototype._loadVersionFromDBAsync = function (url, callback, updateInDBCallback) {
...
    transaction.oncomplete = function (event) {
        if (version) {
        // If the version in the JSON file is > than the version in DB
            if (_this.manifestVersionFound > version.data) {
                _this.mustUpdateRessources = true;

Notice that the version number comparison operator is >, not !=. Shouldn't the comparison be != to cover any changes to the .manifest file version, regardless of its age? I've changed this on my end, and all is well now.

Link to comment
Share on other sites

It's not a bug, it's the expected result.

This is because the versions of the manifest are supposed to be incrementing. using! != this could also allow to decrement, which is not consistent with a version system. 

But maybe Deltakosh could have an idea for your particular case of being able to go back to an old version put cache.

Link to comment
Share on other sites

31 minutes ago, Dad72 said:

It's not a bug, it's the expected result.

This is because the versions of the manifest are supposed to be incrementing. using! != this could also allow to decrement, which is not consistent with a version system. 

But maybe Deltakosh could have an idea for your particular case of being able to go back to an old version put cache.

While it may not be technically consistent with a version system, I would argue that it's actually vital to a versioning system. All versioning systems allow the ability to roll back changes, which in this case breaks the operation of the engine if those rollbacks include important changes to scene files. It would be like Unity insisting that it load the absolute latest version of all assets, regardless of which version/branch/fork of the game you happen to be working on.

Also, I have thousands of users who sometimes test new versions of the game, only to return to the old one while I make changes. They need a way for their browsers to seamlessly retrieve the appropriate assets for both, regardless of which way they're moving through the versioning chain. It would be essentially impossible for me to explain to all of them that they need to open up their developer console, search for the babylonjs IndexedDB and delete it when they're done testing.

Link to comment
Share on other sites

It was designed to work with incrementing version.

That's being said, I get your point and we probably can add a boolean which force the system to do a !== instead of >. If you can do the PR today I should be able to merge it for 3.2 (Which is due this friday)

Link to comment
Share on other sites

9 hours ago, Deltakosh said:

It was designed to work with incrementing version.

That's being said, I get your point and we probably can add a boolean which force the system to do a !== instead of >. If you can do the PR today I should be able to merge it for 3.2 (Which is due this friday)

Will do, but I doubt I'll be able to get to it before the merge. I'm going to continue to stubbornly stand by my opinion that this should be the default behavior, though :). I could be swayed if someone were to lay out a scenario where this would break something, but I can't for the life of me think of one myself. I have, however, witnessed first hand how the current behavior does.

Link to comment
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.
Note: Your post will require moderator approval before it will be visible.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

Loading...
 Share

  • Recently Browsing   0 members

    • No registered users viewing this page.
×
×
  • Create New...