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

Add BMP test nodes #13934

Closed
wants to merge 1 commit into from
Closed

Conversation

erlehmann
Copy link
Contributor

@erlehmann erlehmann commented Oct 27, 2023

Goal of the PR

  • Test BMP rendering
  • Test BMP deprecation detection

How does the PR work?

This patch adds two test nodes for the BMP texture format:

  • a 24bpp test node that loads a 2×2 RGB bitmap from a file
  • a 24bpp test node that loads a base64-encoded in-band 2×2 RGB bitmap

Does it resolve any reported issue?

This patch is adjacent to this issue: #13918
It tests the BMP deprecation feature from this PR: #13922

The test nodes are intended to check that BMP deprecation detection works.
They also have a hint in the description that BMP textures are deprecated (just to make it very clear).

Does this relate to a goal in the roadmap?

Correctness:

  • BMP rendering is tested (for 24bpp RGB only)
  • BMP deprecation warnings are tested (for both loading a BMP from file and inline base64-encoded BMPs).

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

It provides 2 test cases for the code in this PR: #13922

Even though the code in that PR seems trivial, the tests are also trivial.

Also I was the only person asking for tests and so it is just fair that I created and added them.

How to test

  1. Start Minetest with the commit from this PR.
  2. Start a devtest game.
  3. Right click while having the ”Bag of Everything” selected.
  4. Search for “BMP” in the formspec.
  5. Acquire and place both test nodes.
  6. Count the number of BMP deprecation warnings Minetest outputs. I think that number should be 2 (but I am not sure).
  7. Verify that the nodes look a bit like the screenshot below (ignore the colored artifacts at the edges please). Each test node should have a 2×2 texture on all sides with the colors being red, green, yellow, blue (going clockwise from top left pixel).

minetest-580-dev-bmp-testnodes

This patch adds two test nodes for the BMP texture format:

- a 24bpp test node that loads a 2×2 RGB bitmap from a file
- a 24bpp test node that loads a base64-encoded in-band 2×2 RGB bitmap

The test nodes are intended to check that BMP deprecation detection works.
They also have a hint in the description that BMP textures are deprecated.
@rubenwardy
Copy link
Member

Thanks for testing. Given BMPs are deprecated, I don't think it's worth committing these to devtest

@erlehmann
Copy link
Contributor Author

erlehmann commented Oct 27, 2023

@rubenwardy okay. Did you actually verify that the deprecation code outputs two warnings though?

Edit: Please close this PR only after you verify that the BMP deprecation detection actually works correctly.

Edit 2: I'm compiling Minetest right now to see if that is the case. If everything works, I will close this PR myself afterwards.

Edit 3: I have two nodes, but I only get one warning. I think this proves that the warning added in #13922 is not enough and there should also be one in the BMP decoder part of IrrlichtMT. Since Minetest's core devs rejected other test nodes that demonstrated bugs in the past, I fully expect this to be closed anyway.

@wsor4035 wsor4035 closed this Oct 27, 2023
@rubenwardy
Copy link
Member

sfan5 will have been aware that his deprecation warning is based on file extension and not on the actual content. The point of deprecation warnings is to notify the modder, they don't need to catch every single edge case

@erlehmann
Copy link
Contributor Author

sfan5 will have been aware that his deprecation warning is based on file extension and not on the actual content. The point of deprecation warnings is to notify the modder, they don't need to catch every single edge case

I understand that, but the problem isI did not find any mods that tried to load valid BMP files. The only ones found so far were doing something dynamic with it. So the ”edge case” is the only case I can not exclude. I have understood now though that me insisting on test cases for other people's code is not welcome even when I provide those tests, so I will probably not do it again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants