Split size into separate width and height values#270
Conversation
render_utils.mjs NOT FINISHED
probably not done, still needs to be tested.
ptrlrd
left a comment
ptrlrd
left a comment
There was a problem hiding this comment.
I'll fully look into this tomorrow, but I see a few areas that can be improved.
Like gl.viewport(0, 0, outputWidth, outputWidth) the second bar should be outputHeight.
I'll do a full review tomorrow. It's super close though
|
|
||
| function renderSlotBySlot(skeleton, renderSize, scale, cx, cy) { | ||
| const compPixels = new Uint8ClampedArray(renderSize * renderSize * 4); | ||
| function renderSlotBySlot(skeleton, renderWidth, renderHeight, scale, cx, cy) { |
There was a problem hiding this comment.
Line 64 is returning 5 args but this has 6 args.
Renderheigth will returned undefined.
| ); | ||
|
|
||
| gl.viewport(0, 0, outputSize, outputSize); | ||
| gl.viewport(0, 0, outputWidth, outputWidth); |
There was a problem hiding this comment.
I think this may be a copy/paste error, second arg should be outputHeight
| for (let y = 0; y < outputSize; y++) { | ||
| flipped.set(pixels.subarray((outputSize - 1 - y) * rowSize, (outputSize - y) * rowSize), y * rowSize); | ||
| const rowSize = outputWidth * 4; | ||
| for (let y = 0; y < outputWidth; y++) { |
There was a problem hiding this comment.
I think this and line R221 should be outputHeight, I think you're flipping y loop here?
| } | ||
|
|
||
| if (bufferOk && nonTransparent > renderSize * renderSize * 0.01) { | ||
| if (bufferOk && nonTransparent > renderWidth * renderWidth * 0.01 && nonTransparent > renderHeight * renderHeight * 0.01) { |
There was a problem hiding this comment.
I think this can be simplified to just nonTransparent >
renderWidth * renderHeight * 0.01 instead of squaring everything
| const sh = maxY - minY; | ||
| const padding = outputSize * 0.05; | ||
| const avail = outputSize - padding * 2; | ||
| // TODO: Generalize this. |
There was a problem hiding this comment.
I think the generalization could prob just be
Math.min(RENDER_WIDTH, RENDER_HEIGHT)
- PADDING * 2
| const skelDir = path.resolve(process.argv[2] || ""); | ||
| const outputPath = path.resolve(process.argv[3] || "output.gif"); | ||
| const outputSize = parseInt(process.argv[4] || "256"); | ||
| const outputWidth = parseInt(process.argv[4] || "256"); |
There was a problem hiding this comment.
This halves it, try *2ing it? same with r57
There was a problem hiding this comment.
if people do node render_gif.mjs
512 they'll get width 512 and height 256 i thinkmaybe try parseInt(process.argv[5] ||
process.argv[4] || "256")
ptrlrd
left a comment
ptrlrd
left a comment
There was a problem hiding this comment.
Overall, I think this is the right way to go. It makes the tool more flexible.
I think testing strategy to see your outcome would be:
- Regenerate a render with default square size and diff against a pre-pr output. If there's nno pixel diff then there's no regression
- Render same skel with a non-square dem and visually confirm that the render sits inside the overlay
Feel free to make the changes + test and let me know, I can rereview.
|
Overall, almost there. Width x height is missing in some areas :] thanks for contributing |
2 participants
First attempt at splitting these up, may be missing some changes and I've marked a couple places where I still need to do work. I do not know how to test this, so if this could be reviewed that would be great, thank you!