Skip to content
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

using an existing canvas view to create a renderer no longer displays the view in 1.6.1. #982

Closed
agamemnus opened this issue Sep 20, 2014 · 7 comments

Comments

@agamemnus
Copy link
Contributor

Topic... no errors, but it is blank.

@agamemnus
Copy link
Contributor Author

Ok, seems that you switched to "width, height, options" instead of "width, height, option1, option2, ..."

It would be nice if you could tell for typeof and allow for the 1.6.0 format as well. It's an easy change, really... autoDetectRenderer would need to pass arguments, though; e.g.:

PIXI.autoDetectRenderer = function() {
    var args = Array.prototype.slice.call(arguments);
    if (typeof args[0] == "undefined") args[0] = 800; // <--- allow for 0, 0 width/height via undefined instead of !.
    if (typeof args[1] == "undefined") args[1] = 600;

    // BORROWED from Mr Doob (mrdoob.com)
    var webgl = ( function () { try {
                                    var canvas = document.createElement( 'canvas' );
                                    return !! window.WebGLRenderingContext && ( canvas.getContext( 'webgl' ) || canvas.getContext( 'experimental-webgl' ) );
                                } catch( e ) {
                                    return false;
                                }
                            } )();

    function construct (constructor, args) { // <--- http://stackoverflow.com/questions/1606797/use-of-apply-with-new-operator-is-this-possible
        function F() {return constructor.apply(this, args)};
        F.prototype = constructor.prototype;
        return new F();
    }

    return construct (PIXI[(webgl ? "WebGL" : "Canvas") + "Renderer"], args);
};

@GoodBoyDigital
Copy link
Member

Hi @agamemnus
I like your thinking but I think this is not required as its such a simple API change. The code is a little too complex for an edge case. What we could do is simple add an throw an error if the 3rd param is not an object?

@agamemnus
Copy link
Contributor Author

You would be breaking compatibility for all present users, in lieu of adding a few lines of code.

Also, inline error catching is horrible (well, ok, not horrible but not ideal), except for feature detection, if unavoidable otherwise. (at least, have a debug wrapper instead...)

Alternatively you could also move the initialization to an .init function, so you can use .apply normally.

Anyhoo, hope you noticed the other changes -- dynamic string in the return instead of if/else (and code duplication) plus width/height would be allowed to be 0 with a typeof "undefined" check. (or, could be a "number" check, but I can see possibilities for the future where a non-number could be used...)

Edit:

Speaking of inline error catching... I wonder what the case is where running canvas.getContext( 'webgl') or canvas.getContext('experimental-webgl') actually causes an error (instead of null)? And is there any browser that has window.WebGLRenderingContext but not a webgl context?

@englercj
Copy link
Member

We should definately maintain compatability if at all possible (or update the major version if we break it). I don't like generating a dynamic constructor for something this simple though

Better IMHO would just to move to the new prototype and leave in a couple lines of compatability code. Check if the "options" param is a canvas object. If it is, then treat it like the old params where they were all separate.

Something along these lines would be fine:

PIXI.autoDetectRenderer = function(width, height, options) {
   // if no/falsey options passed or a view passed create the options object from arguments.
    if (!options || options && options.nodeName && options.nodeName.toLowerCase() === 'canvas') {
        options = {
            view: arguments[2],
            transparent: arguments[3] || false,
            antialias: arguments[4] || false,
            preserveDrawingBuffer: arguments[5] || false,
            resolution: arguments[6] || 1 // OR 1 is ok here because falsey is an invalid value
        }
    }

    // use options like expected.
}

@agamemnus
Copy link
Contributor Author

Hmm. Well, the only problem with this is that you would need add the same code in the WebGL and Canvas renderer functions, if you wanted to maintain compatibility for all of the functions. If you just pass the arguments you would only have two of that kind of logic.

@englercj
Copy link
Member

This is a moot point, we are breaking backwards compat in the next release.

@lock
Copy link

lock bot commented Feb 26, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked and limited conversation to collaborators Feb 26, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants