c-frame / aframe-stereo-component

aframe.io component to enable separate eye rendering of objects, plus stereo video rendering (full 360 or half dome)
https://c-frame.github.io/aframe-stereo-component/
MIT License
137 stars 52 forks source link

modified checks for string selector to allow for a-assets translation… #21

Closed MagicIndustries closed 7 years ago

MagicIndustries commented 7 years ago

Fixes an issue where a-assets images' selectors were being translated to the full asset path by the time the stereo component was checking to see if the item was a video.

Fix essentially just checks that the string is a valid selector before attempting to use document.querySelector() on it while checking if it's a video tag.

AmadeusW commented 7 years ago

I also have the same issue and this PR helped me a lot. This PR looks good but I have some suggestions. Despite them, this PR needs to be merged. The component is otherwise unusable, and it renders the whole application unusable.

Using try-catch for logic is a bad practice, so I'd try to find a different heuristic before even attempting the querySelector - for example, "does src contain /"?

Alternatively, the code can be shortened to:

// try/catch to validate selector as we might have a string that is actually the
// asset path replaced by the a-assets system IE <a-entity material="src:#imgID">
// may get translated to "/assets/image.jpg"
try {
    if (document.querySelector(src).tagName === "VIDEO"
        || 'tagName' in src && src.tagName === "VIDEO") {
        // If src is a video element , just get the tagName
            this.material_is_a_video = true;
    }
} catch(e) {
}