Proposed fix for issue 3257

[diarmidmackenzie]

Description:

A potential fix for issue 3257, an old bug which came up again recently on the A-Frame slack.

As per @donmccurdy's comment, the problem is with this line of code:
#3257 (comment)

aframe/src/utils/material.js

Line 70 in d27fd57

// Don't process if material src hasn't changed.

// Don't process if material src hasn't changed.
if (src === shader.materialSrcs[materialName]) { return; }

This glitch shows that removing this line appears to solve the issue, therefore I am proposing we simply remove this line, to fix this problem.
https://glitch.com/edit/#!/texture-offset-with-aframe-fix?path=index.html%3A9%3A59

My concern is whether there are circumstances in which this line of code was delivering valuable optimization.

My expectation is that the material update() processing won't be invoked unless there has been a change to attributes, and therefore there will be few (if any) situations where that line of code was actually doing the right thing.

It should be possible to write a more elaborate fix where we explicitly check for changes to the various parameters (offset, repeat etc.), and return if none of them have been changed. However I believe that code is superfluous and would create a maintenance over head.

Hence proposing the simple fix, but happy to implement a more elaborate fix if there are good reasons to do so.

[diarmidmackenzie]

Hmm. Looking at this a bit more, it seems like a better fix might be to call

setTextureProperties()

in this case....

That ought to achieve what needs doing, but without the costs of reloading the texture...

[diarmidmackenzie]

Based on the suggestion above, I think I have improved the fix. Still seems to work ion the problem case we had, and should avoid unecessary repeat loading of textures.

[dmarcos]

Not necessary to commit dist and index.js files

[dmarcos]

Thanks! I think it looks reasonable. I was also concerned with doing extra work when unnecessary.

Perhaps a further improvement would be to check if the values of the texture have changed before setting them again

[dmarcos]

If we remove changes in dist / index file this one is pretty close. Thanks

[diarmidmackenzie]

I probably didn't do that in the most efficient way, but I think I got there. Changes to index.js and dist files now removed from the PR.

[diarmidmackenzie]

I didn't understand the intent of this comment:
"Perhaps a further improvement would be to check if the values of the texture have changed before setting them again"

The things we are setting here are just vec2s? Doesn't seem to deliver any real efficiency gain to check for diffs before setting vs. just setting the new values...?

[dmarcos]

Thanks for the patience! Great effort!