Tweaks to fix gl2 rendering artifacts by HunterZ · Pull Request #114 · libtcod/libtcod

HunterZ · 2022-03-26T18:02:06Z

CMakeLists.txt:
- MinGW compat fix
src/libtcod/renderer_gl.c:
- implement optional atlas padding support
- add more comments
src/libtcod/renderer_gl.h:
- add optional TILE_PADDING macro to configure atlas padding support
src/libtcod/renderer_gl2.c:
- fragment shader: use atlas pixel dimensions instead of row/col index
- fragment shader: remove clamping logic
- fragment shader: use texture2DLod to force disable mipmap use
- encode atlas pixel coordinates in ch buffer texture instead of row/col indexes
- send actual atlas texture size to fragment shader instead of recalculating

- CMakeLists.txt: - MinGW compat fix - src/libtcod/renderer_gl.c: - implement optional atlas padding support - add more comments - src/libtcod/renderer_gl.h: - add optional TILE_PADDING macro to configure atlas padding support - src/libtcod/renderer_gl2.c: - fragment shader: use atlas pixel dimensions instead of row/col index - fragment shader: remove clamping logic - fragment shader: use texture2DLod to force disable mipmap use - encode atlas pixel coordinates in ch buffer texture instead of row/col indexes - send actual atlas texture size to fragment shader instead of recalculating

HexDecimal · 2022-03-26T18:14:10Z

I'm having a hard time understanding TILE_PADDING. I'm expecting a constant like this to be padding per-side.

I wouldn't mind hard-coding a pixel of padding on each side of a tile.

codecov · 2022-03-26T18:16:44Z

Codecov Report

Merging #114 (704e450) into develop (713817b) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

❗ Current head 704e450 differs from pull request most recent head 57719a2. Consider uploading reports for the commit 57719a2 to get more accurate results

@@             Coverage Diff             @@
##           develop     #114      +/-   ##
===========================================
- Coverage    12.56%   12.52%   -0.05%     
===========================================
  Files          124      124              
  Lines        13240    13283      +43     
===========================================
  Hits          1664     1664              
- Misses       11576    11619      +43

Impacted Files	Coverage Δ
src/libtcod/renderer_gl.c	`0.00% <0.00%> (ø)`
src/libtcod/renderer_gl2.c	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 713817b...57719a2. Read the comment docs.

HexDecimal · 2022-03-26T18:20:44Z

clang-tidy should be run on this code before you commit it.

Don't comment out code on version control, it should be deleted or stashed.

HexDecimal · 2022-03-26T18:23:29Z

src/libtcod/renderer_gl.c

+    columns = (new_size - TILE_PADDING) / (atlas->tileset->tile_width + TILE_PADDING);
+    rows = (new_size - TILE_PADDING) / (atlas->tileset->tile_height + TILE_PADDING);


This can't be something simpler? Like columns = new_size / (atlas->tileset->tile_width + TILE_PADDING * 2);?

I assume that the total tile size is the tile size plus the padding.

clang-tidy should be run on this code before you commit it.

Don't comment out code on version control, it should be deleted or stashed.

I warned you this wasn't ready for primetime!

This can't be something simpler? Like columns = new_size / (atlas->tileset->tile_width + TILE_PADDING * 2);?

I assume that the total tile size is the tile size plus the padding.

No. There needs to be padding along one side of each dimension of the atlas, plus per-tile padding along the opposite sides of each dimension of each tile.

The total atlas size in a given dimension is therefore padding_size + numTiles.dimension * (tile_size.dimension + padding_size)

I assume that if the tiles are laid out like this:

111222 111222 111222

A pixel of padding will be applied like this:

########## #111##222# #111##222# #111##222# ##########

But I might be wrong with the exact math. I often program iteratively.

PADDING=2 with my approach, used with 2x2 tiles:

########## ########## ##11##22## ##11##22## ########## ########## ##33##44## ##33##44## ########## ##########

Your suggestion would be less compact:

############ ############ ##11####22## ##11####22## ############ ############ ############ ############ ##33####44## ##33####44## ############ ############

Your approach would allow for odd numbered padding values though, which I didn't consider. Let me know if you think this is worth pursuing.

I'd like to go with my approach and then hardcode a padding value of 1.

Alternatively I'd like to figure out framebuffers so that I can skip padding entirely, but I can try that on my PR.

Fixed. Also verified that (at least with texture2DLod) TILE_PADDING value of 1 works for both GL_NEAREST and GL_LINEAR, and that commenting out the macro still works.

HexDecimal · 2022-03-26T18:25:30Z

samples/CMakeLists.txt

+if(MINGW)
+    set(LINK_TCOD mingw32 "${LINK_TCOD}")
+endif()


I don't understand where this fix comes from. Why is mingw32 hard coded here?

Because when compiling SDL2 with MinGW in the MSYS2 environment, you need to link libmingw32.a first, or else the linker stage will fail. This is a widely-known issue.

HexDecimal · 2022-03-26T18:45:57Z

I warned you this wasn't ready for primetime!

That's not a problem. I'm just pointing it out now so that I don't need to mention it later. I'll handle anything that you miss.

- renderer_gl.c: - assume each tile has its own TILE_PADDING pixels of padding around all sides in the atlas texture - remove commented out code - slight optimizations and comments / parameter name tweaks - renderer_gl.h: - default to a TILE_PADDING value of 1 - renderer_gl2.c: - assume each tile has its own TILE_PADDING pixels of padding around all sides in the atlas texture - remove commented out code - tweak comments

HexDecimal · 2022-11-07T15:28:52Z

Closed because the OpenGL renderers have been removed by #137.

HunterZ mentioned this pull request Mar 26, 2022

Text alignment doesn't seem to work after updating? #83

Closed

HexDecimal marked this pull request as draft March 26, 2022 18:15

HexDecimal reviewed Mar 26, 2022

View reviewed changes

HexDecimal changed the base branch from develop to main August 11, 2022 12:38

HexDecimal mentioned this pull request Nov 7, 2022

Remove OpenGL renderers and SDL software fallback renderer. #137

Merged

HexDecimal closed this Nov 7, 2022

		columns = (new_size - TILE_PADDING) / (atlas->tileset->tile_width + TILE_PADDING);
		rows = (new_size - TILE_PADDING) / (atlas->tileset->tile_height + TILE_PADDING);

Uh oh!

Conversation

HunterZ commented Mar 26, 2022

Uh oh!

HexDecimal commented Mar 26, 2022

Uh oh!

codecov bot commented Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

HexDecimal commented Mar 26, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HunterZ Mar 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HexDecimal commented Mar 26, 2022

Uh oh!

HexDecimal commented Nov 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Mar 26, 2022 •

edited

Loading

HunterZ Mar 26, 2022 •

edited

Loading