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

Better Quality Documentation #42

Merged
merged 15 commits into from
Dec 6, 2017

Conversation

robertmain
Copy link
Contributor

@robertmain robertmain commented Nov 24, 2017

This PR builds on the original docblock documentation(originally proposed in #32) to provide more helpful information to consumers of the API. In particular, it makes very heavy references to MDN - however in each case, a link has been provided to the relevant MDN page(this will be displayed in the generated documentation). In particular this should be very useful when it's auto-generated (see #39) for each commit to master.

However - I do have a few questions regarding some of the methods and/or parameters whose purpose I wasn't easily able to determine from reading the code and/or MDN...

If you're able to answer these questions I can update the PR with the relevant docs..

Context.js

  1. fillPixelWithColor - x and y kinda make sense...but what does col do? How is it used and how should I go about documenting it's use?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L322-L328
    fillPixelWithColor(i,j,col) {
        if(!this.pixelInsideClip(i,j)) return;
        var new_pixel = col;
        var old_pixel = this.bitmap.getPixelRGBA(i,j);
        var final_pixel = this.composite(i,j,old_pixel,new_pixel);
        this.bitmap.setPixelRGBA(i,j,final_pixel);
    }
  2. composite - What do the oldpixel and newpixel parameters mean? Also - the first two parameters are never actually used... can this be changed?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L342-L362
    composite(i,j,old_pixel, new_pixel) {
        const old_rgba = uint32.getBytesBigEndian(old_pixel);
        const new_rgba = uint32.getBytesBigEndian(new_pixel);
    
    
        //convert to range of 0->1
        const A = new_rgba.map((b)=>b/255);
        const B = old_rgba.map((b)=>b/255);
        //multiply by global alpha
        A[3] = A[3]*this._globalAlpha;
    
    
        //do a standard composite (SRC_OVER)
        function compit(ca,cb,aa,ab) {
            return (ca*aa + cb*ab * (1-aa)) / (aa+ab*(1-aa));
        }
        const C = A.map((comp,i)=> compit(A[i],B[i],A[3],B[3]));
    
    
        //convert back to 0->255 range
        const Cf = C.map((c)=>c*255);
        //convert back to int
        return uint32.fromBytesBigEndian(Cf[0],Cf[1],Cf[2],Cf[3]);
    }
  3. calculateRGBA - It looks like this accepts parameters, but never actually uses them - can we get rid of those?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L374-L376
     calculateRGBA(x,y) {
         return this._fillColor;
     }
  4. calculateRGBA_stroke - It looks like this accepts parameters, but never actually uses them - can we get rid of those?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L388-L390
    calculateRGBA_stroke(x,y) {
        return this._strokeColor;
    }
  5. calcQuadraticAtT
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L1056-L1060
    function calcQuadraticAtT(p, t) {
        var x = (1-t)*(1-t)*p[0].x + 2*(1-t)*t*p[1].x + t*t*p[2].x;
        var y = (1-t)*(1-t)*p[0].y + 2*(1-t)*t*p[1].y + t*t*p[2].y;
        return new Point(x, y);
    }
  6. getImageData - It looks like this accepts parameters, but never actually uses them - can we get rid of those?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L405-L407
    getImageData(x,y,w,h) {
        return this.bitmap;
    }
  7. calcMinimumBounds - Need a better description of this
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L1083-L1096
    function calcMinimumBounds(lines) {
        var bounds = {  x:  Number.MAX_VALUE, y:  Number.MAX_VALUE,  x2: Number.MIN_VALUE, y2: Number.MIN_VALUE }
        function checkPoint(pt) {
            bounds.x  = Math.min(bounds.x,pt.x);
            bounds.y  = Math.min(bounds.y,pt.y);
            bounds.x2 = Math.max(bounds.x2,pt.x);
            bounds.y2 = Math.max(bounds.y2,pt.y);
        }
        lines.forEach(function(line) {
            checkPoint(line.start);
            checkPoint(line.end);
        })
        return bounds;
    }
  8. calcSortedIntersections - What does the y parameter do? Does it represent the actual y axis, or is it just a letter?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L1111-L1122
    function calcSortedIntersections(lines,y) {
        var xlist = [];
        for(var i=0; i<lines.length; i++) {
            var A = lines[i].start;
            var B = lines[i].end;
            if(A.y<y && B.y>=y || B.y<y && A.y>=y) {
                var xval = A.x + (y-A.y) / (B.y-A.y) * (B.x-A.x);
                xlist.push(xval);
            }
        }
        return xlist.sort(function(a,b) {  return a-b; });
    }
  9. lerp - What do these params mean?
    https://github.com/robertmain/node-pureimage/blob/master/src/context.js#L1139
    function lerp(a,b,t) {  return a + (b-a)*t; }
    

Bitmap.js

  1. constructor - It's asking for the options parameter, but this seems to never actually be used...
    https://github.com/robertmain/node-pureimage/blob/master/src/bitmap.js#L20-L44

    constructor(w,h, options) {
        /**
         * @type {number}
         */
        this.width = Math.floor(w);
    
        /**
         * @type {number}
         */
        this.height = Math.floor(h);
    
        /**
         * @type {ArrayBuffer}
         */
        this.data = Buffer.alloc(w*h*4);
    
        var fillval = NAMED_COLORS.black;
        for(var j=0; j<h; j++) {
            for (var i = 0; i < w; i++) {
                this.setPixelRGBA(i, j, fillval);
            }
        }
    }
  2. getPixelRGBA_separate - Can you help me come up with a description of this method? Maybe an example that I could include in the docs?

    getPixelRGBA_separate(x,y) {
        var i = this.calculateIndex(x,y);
        return this.data.slice(i,i+4);
    }

@joshmarinacci
Copy link
Owner

Most of these are not part of the HTML Canvas spec and should not be documented or exposed. Is there a way to make the doc generator skip certain methods?

getImageData is part of the spec. It is supposed to return a subset of the underlying image, which this code doesn't do yet (obviously). So that's a bug (or technically, an unimplemented api function).

@robertmain
Copy link
Contributor Author

Sure is - I can add @ignore before the ones you don't want published if you let me know which ones those are (or did you just means all of the ones I listed above)?

@joshmarinacci
Copy link
Owner

joshmarinacci commented Nov 24, 2017 via email

@robertmain
Copy link
Contributor Author

OK - I added @ignore to the API methods you specified (except the constructor for Bitmap of course - see 8f534ca . That said - perhaps it might still be helpful to have docs for those methods anyway just for library contributors to refer to? :)

Remove excess spacing from the end of lines etc.
Move class properties out of `Object.defineProperty` calls to class getters and setters. This allows us to target them with esdoc annotations in a way that wasn't possible before with `Object.defineProperty`

Ref joshmarinacci#32
- Remove `/** @ignore*/` before each dependency require - unfortunately this will make the documentation coverage score go down, but it seems better to have easier to read code
- Assign dependencies with `const` instead of `var` since it prevents accidental re-assignment
- Sort dependencies alphabetically
Add `@ignore` to unimplemented methods to hide them from the documentation

Ref joshmarinacci#32
- Hide private methods and properties
- Add `esdoc-member-plugin` to document ES6 class getters and setters

Ref joshmarinacci#32
- Update documentation for line related functions to provide examples on how they can be called
- Also remove `makeLine()` function since it is now obsolete

Ref joshmarinacci#32
Update `Context` to more closely match the documentation on MDN, since it is intended to match the API available from the HTML5 `<canvas>` API. In each case - a link has been provided to the appropriate MDN page, both for attribution and for further reference.

Ref joshmarinacci#32
Provide documentation for additional methods(in particular those not available on MDN for reference) to better explain their purpose and usage. Whilst some of these methods may be private and/or non-exported - this information may still prove useful for library contributors and maintainers(some IDEs can provide this information in a popup dialog box).

Also, make more usage of `{@link }` to link to other classes, functions, variables etc. when talking about them in the docblocks

Ref joshmarinacci#32
- Align `=` signs to make the assignments easier to read
- Replace single-line `if` statements to wrap `return` in braces so it's easier to read at a glance
- Remove commented out `console.log` logging/debugging code
Fix small documentation bugs(missing asterisks etc.) causing impoper formatting of documentation

Ref joshmarinacci#32
Add missing docblocks for `start` and `end` attribues of `Line`

Ref joshmarinacci#32
Provide more detailed documentation on the Bitmap class

Ref joshmarinacci#32
Add `@ignore` tag to methods that are not part of the canvas API spec

Ref joshmarinacci#32
Update function docblocks to correctly reflect the fact that resolved
promises resolve to `void`

Ref joshmarinacci#32
@joshmarinacci joshmarinacci merged commit f3df4a2 into joshmarinacci:master Dec 6, 2017
@robertmain robertmain deleted the documentation-tidy-up branch December 6, 2017 17:25
@robertmain robertmain mentioned this pull request Dec 6, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants