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

WIP: Devtest: Add oddly sized checkerboard test nodes #13949

Conversation

erlehmann
Copy link
Contributor

@erlehmann erlehmann commented Nov 1, 2023

Goal of the PR

I want to test display and scaling of textures with dimensions other than 16×16.

Existing devtest nodes are limited to a few common texture sizes, listed below:

; identify games/devtest/mods/testnodes/textures/*.png |cut -f3 -d' ' |sort |uniq
16x16
16x2
16x64
32x32
512x512
64x1
64x64
8x1
8x32

How does the PR work?

The code in the patch generates 25 checkerboard textures in different sizes and registers nodes for them in devtest.

Edit: After a conversation on IRC with pgimeno I am looking for better test patterns than checkerboard. Suggestions?

Edit 2: pgimeno pointed me to a proposal for better texture upscaling of pixel art that could be tested with new nodes..

Does it resolve any reported issue?

Not at all. I mean … I found some issues using these nodes, but have not reported or checked if they were reported.

Does this relate to a goal in the roadmap?

Correctness. These nodes can be used to check issues with handling of non-16×16 textures sizes and texture scaling.

If not a bug fix, why is this PR needed? What usecases does it solve?

These nodes can be useful to fiind and fix bugs relating to handling of non-16×16 textures sizes and texture scaling.

How to test

  1. Start devtest.
  2. Open the bag with everything (or how it is called).
  3. Type “Checkerboard” in the search field and press the return key.
  4. Look at the nodes. Is the inventory rendering okay? How do they look in the world? Anything weird?

Screenshots

Edit: Look at the screenshots at full size to avoid browser-related texture scaling issues.

Inventory rendering shows weird patterns depending on texture size. Could this be improved?

minetest-game-devtest-testnodes-checkerboard

Some rendering artifacts at node edges that only seem to happen with some texture sizes.

minetest-game-devtest-testnodes-checkerboard-2

The same scene, but without the rendering artifacts. It suggests that these artifacts are a driver issue.

minetest-game-devtest-testnodes-checkerboard-3

The artifact in this screenshot is probably Minetest's fault. These kind of things just happen:

minetest-game-devtest-testnodes-checkerboard-4

Edit: Anisotropic filtering reduces scaling artifacts for big textures somewhat, but seems to mess up small textures badly:

minetest-game-devtest-testnodes-checkerboard-5-anisotropic

@Zughy Zughy added Feature ✨ PRs that add or enhance a feature @ Devtest Game Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand. @ Client / Audiovisuals labels Nov 1, 2023
@erlehmann
Copy link
Contributor Author

erlehmann commented Nov 1, 2023

@Zughy why is this tagged ”Roadmap: Needs approval”? I mean it needs approval to be merged anyway, but I had the impression that this falls under 2.1 Rendering/Graphics improvements … as in “having devtest nodes to check if texture scaling works okay is a prerequisite for finding and fixing bugs related to texture scaling“.

@Zughy
Copy link
Member

Zughy commented Nov 1, 2023

Because it's up to core devs to understand whether they want to add such feature to tackle rendering issues or, on the contrary, they want to proceed in a different way. Unsubscribing

@erlehmann
Copy link
Contributor Author

@Zughy this is a test case, not a feature. No one is ever going to use these things outside of debugging. I think the tag ”Feature” is inappropriate for stuff that is never exposed in the API and can only ever used to test if an actual feature (e.g. mipmapping or anisotropic filtering) works of if a change to it improves things.

Also, just for my understanding, can I just write “Unsubscribing” at the end of the message so that I do not get notified for an issue/PR unless someone tags me (I assume that is what it does, bar further information) or do I have to do something in the GitHub interface and this is just to let others now? I sometimes get notified for threads in which I commented once and I wonder.

@Zughy
Copy link
Member

Zughy commented Nov 2, 2023

It's something new being added, hence to me it's a feature. Core devs are free to change labels if they think I'm wrong

About unsubscribing: no, it's just a way for me to communicate that I won't receive any notifications unless tagged again. I manually press "Unsubscribe" (..unsubscribing)

@erlehmann
Copy link
Contributor Author

It's something new being added, hence to me it's a feature.

Oh, now i get it. I mostly think in terms of ”will this make a difference to the end user” and the nodes make about zero difference.

it's just a way for me to communicate that I won't receive any notifications unless tagged again. I manually press "Unsubscribe" (..unsubscribing)

@Zughy sorry to tag you once more, but one more question: Does GitHub not show at all who will be notified? (I can't find it.)

@Zughy
Copy link
Member

Zughy commented Nov 2, 2023

Pretty sure it doesn't

@erlehmann erlehmann changed the title Devtest: Add oddly sized checkerboard test nodes WIP: Devtest: Add oddly sized checkerboard test nodes Nov 2, 2023
@erlehmann
Copy link
Contributor Author

Set to WIP because I am going to make some better test textures.

@erlehmann
Copy link
Contributor Author

I think I'm going to redo this entire thing. So closing instead of editing it.

@erlehmann erlehmann closed this Nov 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@ Client / Audiovisuals @ Devtest Game Feature ✨ PRs that add or enhance a feature Roadmap: Needs approval The change is not part of the current roadmap and needs to be approved by coredevs beforehand.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants