Mineclonia
/
Mineclonia
Archived
12
17
Fork 13
Code Issues 105 Pull Requests 19 Projects Releases Wiki Activity

Render preview banners with transparent patterns correctly #72

Closed
erlehmann wants to merge 3 commits from banner-mask-fix-3 into master
pull from: banner-mask-fix-3
merge into: Mineclonia:master
Conversation 10 Commits 3 Files Changed 1 +4 -2
erlehmann commented 2021-05-17 15:54:50 +02:00
Owner
Copy Link

Without this fix, the banner pattern preview generation does not mask
the banner pattern, so the alpha channel of the banner pattern is not
taken into account. This lead to preview banners with color gradients
showing up as a solid color banner and opaque pixel artifacts for the
bottom triangle pattern.

How to test

As a reviewer, please copy the entire section and check what you have tested. Even partial reviews help!

With this patch applied, craft banners in survival mode with patterns on a crafting table. A preview banner should appear in the crafting result preview field of the crafting table. The preview banner should have a light gray background with a downscaled version of the new banner pattern. Verify that the previewed is correct by actually crafting the banner and placing it in the world.

  • Craft an empty white banner item with a crafting table using one stick and six white wool.
  • Craft an empty yellow banner item with a crafting table using one stick and six yellow wool.

Verify that the following preview banners are rendered correctly (patterns ingredients in parentheses):

  • no pattern (no dye)
  • white border(8 bone meal)
  • light grey circle (1 light grey dye)
  • dark grey creeper (1 grey dye + creeper head)
  • violet cross / x (5 purple dye)
  • blue curly border (1 lapis lazuli + 1 vine)
  • light blue diagonal (3 light blue dye)
  • cyan flower (1 cyan dye + 1 oxeye daisy)
  • dark green gradient (4 cactus green)
  • green reverse gradient (4 lime dye)
  • yellow cross / + (5 dandelion yellow)
  • brown chevron / bottom triangle (4 cocoa beans)
  • orange inverted chevron / top triangle (4 orange dye)
  • red triangles bottom (3 rose red)
  • magenta triangles top (3 magenta dye)
  • pink brick wall (1 pink dye + 1 brick block)

Verify that at least one banner wtih a partially transparent motive can be placed and looks correct. Mention in your review which banner you placed.

Without this fix, the banner pattern preview generation does not mask the banner pattern, so the alpha channel of the banner pattern is not taken into account. This lead to preview banners with color gradients showing up as a solid color banner and opaque pixel artifacts for the bottom triangle pattern. ## How to test As a reviewer, please copy the entire section and check what you have tested. Even partial reviews help! With this patch applied, craft banners *in survival mode* with patterns on a crafting table. A preview banner should appear in the crafting result preview field of the crafting table. The preview banner should have a light gray background with a downscaled version of the new banner pattern. Verify that the previewed is correct by actually crafting the banner and placing it in the world. - [ ] Craft an empty white banner item with a crafting table using one stick and six white wool. - [ ] Craft an empty yellow banner item with a crafting table using one stick and six yellow wool. Verify that the following preview banners are rendered correctly (patterns ingredients in parentheses): - [ ] no pattern (no dye) - [ ] white border(8 bone meal) - [ ] light grey circle (1 light grey dye) - [ ] dark grey creeper (1 grey dye + creeper head) - [ ] violet cross / x (5 purple dye) - [ ] blue curly border (1 lapis lazuli + 1 vine) - [ ] light blue diagonal (3 light blue dye) - [ ] cyan flower (1 cyan dye + 1 oxeye daisy) - [ ] dark green gradient (4 cactus green) - [ ] green reverse gradient (4 lime dye) - [ ] yellow cross / + (5 dandelion yellow) - [ ] brown chevron / bottom triangle (4 cocoa beans) - [ ] orange inverted chevron / top triangle (4 orange dye) - [ ] red triangles bottom (3 rose red) - [ ] magenta triangles top (3 magenta dye) - [ ] pink brick wall (1 pink dye + 1 brick block) Verify that at least one banner wtih a partially transparent motive can be placed and looks correct. Mention in your review which banner you placed.
erlehmann added 1 commit 2021-05-17 15:54:51 +02:00
0b65c0c9ae
Render preview banners with transparent patterns correctly 
Without this fix, the banner pattern preview generation does not mask
the banner pattern, so the alpha channel of the banner pattern is not
taken into account. This lead to preview banners with color gradients
showing up as a solid color banner and opaque pixel artifacts for the
bottom triangle pattern.
erlehmann commented 2021-05-17 15:55:20 +02:00
Author
Owner
Copy Link

This replaces #70 – I will now run the tests myself.

This replaces https://git.minetest.land/Mineclonia/Mineclonia/pulls/70 – I will now run the tests myself.
e commented 2021-05-17 22:46:42 +02:00
Member
Copy Link

@erlehmann Just to clarify, do you mean to apply each of these patterns to the original pair of banners, or to make a new pair for each of these patterns?

@erlehmann Just to clarify, do you mean to apply each of these patterns to the original pair of banners, or to make a new pair for each of these patterns?
e requested changes 2021-05-17 23:07:53 +02:00
e left a comment
Member

Passed: (Base)

  • Craft an empty white banner item with a crafting table using one stick and six white wool.
  • Craft an empty yellow banner item with a crafting table using one stick and six yellow wool.

Passed: (First batch)

  • no pattern (no dye)
  • white border(8 bone meal)
  • light grey circle (1 light grey dye)
  • dark grey creeper (1 grey dye + creeper head)
  • violet cross / x (5 purple dye)
  • blue curly border (1 lapis lazuli + 1 vine)
  • light blue diagonal (3 light blue dye)
  • cyan flower (1 cyan dye + 1 oxeye daisy)
  • dark green gradient (4 cactus green)

Passed: (Second batch)

  • green reverse gradient (4 lime dye)
  • yellow cross / + (5 dandelion yellow)
  • brown chevron / bottom triangle (4 cocoa beans)

Failed: Banners no longer accepted dyes after this point

  • orange inverted chevron / top triangle (4 orange dye)
  • red triangles bottom (3 rose red)
  • magenta triangles top (3 magenta dye)
  • pink brick wall (1 pink dye + 1 brick block)
------ **Passed: (Base)** - [x] Craft an empty white banner item with a crafting table using one stick and six white wool. - [x] Craft an empty yellow banner item with a crafting table using one stick and six yellow wool. ------ **Passed: (First batch)** - [x] no pattern (no dye) - [x] white border(8 bone meal) - [x] light grey circle (1 light grey dye) - [x] dark grey creeper (1 grey dye + creeper head) - [x] violet cross / x (5 purple dye) - [x] blue curly border (1 lapis lazuli + 1 vine) - [x] light blue diagonal (3 light blue dye) - [x] cyan flower (1 cyan dye + 1 oxeye daisy) - [x] dark green gradient (4 cactus green) ------ **Passed: (Second batch)** - [x] green reverse gradient (4 lime dye) - [x] yellow cross / + (5 dandelion yellow) - [x] brown chevron / bottom triangle (4 cocoa beans) **Failed: Banners no longer accepted dyes after this point** - [ ] orange inverted chevron / top triangle (4 orange dye) - [ ] red triangles bottom (3 rose red) - [ ] magenta triangles top (3 magenta dye) - [ ] pink brick wall (1 pink dye + 1 brick block)
erlehmann commented 2021-05-18 00:44:17 +02:00
Author
Owner
Copy Link

Just to clarify, do you mean to apply each of these patterns to the original pair of banners, or to make a new pair for each of these patterns?

I did not think about that actually. :(

> Just to clarify, do you mean to apply each of these patterns to the original pair of banners, or to make a new pair for each of these patterns? I did not think about that actually. :(
erlehmann commented 2021-05-18 00:44:55 +02:00
Author
Owner
Copy Link

@e what exactly do you mean with (first batch) and (second batch)?

@e what exactly do you mean with `(first batch)` and `(second batch)`?
e commented 2021-05-18 05:59:30 +02:00
Member
Copy Link

@erlehmann Crafted banners are limited to some number of layers (8?). I assumed your test meant to put as many test patterns on each pair of banners as possible. I crafted 4 banners (2 yellow, 2 white), and after the first pair stopped accepting new patterns, I started using the second pair. The first pair stopped accepting patterns after the dark green pattern. The second pair stopped accepting new patterns after adding the brown chevron, which seems wrong.

@erlehmann Crafted banners are limited to some number of layers (8?). I assumed your test meant to put as many test patterns on each pair of banners as possible. I crafted 4 banners (2 yellow, 2 white), and after the first pair stopped accepting new patterns, I started using the second pair. The first pair stopped accepting patterns after the dark green pattern. The second pair stopped accepting new patterns after adding the brown chevron, which seems wrong.
erlehmann commented 2021-05-18 21:41:26 +02:00
Author
Owner
Copy Link

@e I found the following in mods/ITEMS/mcl_banners/patterncraft.lua:

	-- Lower layer limit when banner includes any gradient.
	-- Workaround to circumvent Minetest bug (https://github.com/minetest/minetest/issues/6210)
	-- TODO: Remove this restriction when bug #6210 is fixed.
	if #layers >= max_layers_gradient then
		for l=1, #layers do
			if layers[l].pattern == "gradient" or layers[l].pattern == "gradient_up" then
				return ItemStack("")
			end
		end
	end

Seems https://github.com/minetest/minetest/issues/6210 was fixed though … this must be made a new issue!

@e I think the issue is not with my PR, do you agree?

@e I found the following in `mods/ITEMS/mcl_banners/patterncraft.lua`: ``` -- Lower layer limit when banner includes any gradient. -- Workaround to circumvent Minetest bug (https://github.com/minetest/minetest/issues/6210) -- TODO: Remove this restriction when bug #6210 is fixed. if #layers >= max_layers_gradient then for l=1, #layers do if layers[l].pattern == "gradient" or layers[l].pattern == "gradient_up" then return ItemStack("") end end end ``` Seems https://github.com/minetest/minetest/issues/6210 was fixed though … this must be made a new issue! @e I think the issue is not with my PR, do you agree?
erlehmann commented 2021-05-18 21:46:54 +02:00
Author
Owner
Copy Link

@e do you think I need to change something here?

@e do you think I need to change something here?
e commented 2021-05-19 02:14:24 +02:00
Member
Copy Link

@erlehmann Thanks for looking in to that. The same behavior is indeed visible on master, and I've filed #73 accordingly. It should be a fairly quick PR, so I've assigned it to myself.

In the meantime, I'll continue testing this PR with a third batch of banners.

@erlehmann Thanks for looking in to that. The same behavior is indeed visible on master, and I've filed #73 accordingly. It should be a fairly quick PR, so I've assigned it to myself. In the meantime, I'll continue testing this PR with a third batch of banners.
e approved these changes 2021-05-19 02:29:36 +02:00
e left a comment
Member

Passed: (Base)

  • Craft an empty white banner item with a crafting table using one stick and six white wool.
  • Craft an empty yellow banner item with a crafting table using one stick and six yellow wool.

Passed: (First batch)

  • no pattern (no dye)
  • white border(8 bone meal)
  • light grey circle (1 light grey dye)
  • dark grey creeper (1 grey dye + creeper head)
  • violet cross / x (5 purple dye)
  • blue curly border (1 lapis lazuli + 1 vine)
  • light blue diagonal (3 light blue dye)
  • cyan flower (1 cyan dye + 1 oxeye daisy)
  • dark green gradient (4 cactus green)

Passed: (Second batch)

  • green reverse gradient (4 lime dye)
  • yellow cross / + (5 dandelion yellow)
  • brown chevron / bottom triangle (4 cocoa beans)

Passed: (Third batch)

  • orange inverted chevron / top triangle (4 orange dye)
  • red triangles bottom (3 rose red)
  • magenta triangles top (3 magenta dye)
  • pink brick wall (1 pink dye + 1 brick block)
------ **Passed: (Base)** - [x] Craft an empty white banner item with a crafting table using one stick and six white wool. - [x] Craft an empty yellow banner item with a crafting table using one stick and six yellow wool. ------ **Passed: (First batch)** - [x] no pattern (no dye) - [x] white border(8 bone meal) - [x] light grey circle (1 light grey dye) - [x] dark grey creeper (1 grey dye + creeper head) - [x] violet cross / x (5 purple dye) - [x] blue curly border (1 lapis lazuli + 1 vine) - [x] light blue diagonal (3 light blue dye) - [x] cyan flower (1 cyan dye + 1 oxeye daisy) - [x] dark green gradient (4 cactus green) ------ **Passed: (Second batch)** - [x] green reverse gradient (4 lime dye) - [x] yellow cross / + (5 dandelion yellow) - [x] brown chevron / bottom triangle (4 cocoa beans) ------ **Passed: (Third batch)** - [x] orange inverted chevron / top triangle (4 orange dye) - [x] red triangles bottom (3 rose red) - [x] magenta triangles top (3 magenta dye) - [x] pink brick wall (1 pink dye + 1 brick block)
e reviewed 2021-05-19 02:32:53 +02:00
mods/ITEMS/mcl_banners/init.lua
@ -389,2 +389,3 @@
local layer = "(([combine:20x40:-2,-2="..pattern.."^[resize:16x24^[colorize:"..color..":"..layer_ratio.."))"
local layer = "(([combine:20x40:-2,-2=" .. pattern .. "^[resize:16x24^[colorize:" .. color .. ":" .. layer_ratio .. "))"
local mask = "(([combine:20x40:-2,-2=" .. pattern .. "^[resize:16x24" .. "))"
e commented 2021-05-19 02:32:52 +02:00
Member

I don't think it's necessary to use two parentheses for either layer or mask here. Probably not worth a commit to fix them, though.

I don't think it's necessary to use two parentheses for either `layer` or `mask` here. Probably not worth a commit to fix them, though.
erlehmann commented 2021-05-30 01:20:16 +02:00
Author
Owner

Please elaborate on that and propose an alternative. I just cargo culted it until it worked.

Please elaborate on that and propose an alternative. I just cargo culted it until it worked.
e commented 2021-05-30 01:58:20 +02:00
Member

layer and mask both start with (( and end with )), despite having no intervening parentheses, meaning (at least) one pair of parens is unneeded.

`layer` and `mask` both start with `((` and end with `))`, despite having no intervening parentheses, meaning (at least) one pair of parens is unneeded.
e commented 2021-05-19 03:11:42 +02:00
Member
Copy Link

A screenshot of the combined banners is attached. From left to right:

  1. Batch 1, yellow base
  2. Batch 1, white base
  3. Batch 2, yellow base
  4. Batch 2, white base
  5. Batch 3, yellow base
  6. Batch 3, white base
A screenshot of the combined banners is attached. From left to right: 1. Batch 1, yellow base 2. Batch 1, white base 3. Batch 2, yellow base 4. Batch 2, white base 5. Batch 3, yellow base 6. Batch 3, white base
Screenshot from 2021-05-18 20-21-53.png
182 KiB
Screenshot from 2021-05-18 20-21-53.png
e added the
bug
 label 2021-05-19 03:19:11 +02:00
erlehmann added 1 commit 2021-05-28 00:49:23 +02:00
erlehmann added 1 commit 2021-05-30 01:18:33 +02:00
erlehmann commented 2021-10-31 21:30:13 +01:00
Author
Owner
Copy Link

Closed in favor of rebased and fixed PR: #158

Closed in favor of rebased and fixed PR: https://git.minetest.land/Mineclonia/Mineclonia/pulls/158
erlehmann closed this pull request 2021-10-31 21:30:14 +01:00
This repo is archived. You cannot comment on pull requests.
Reviewers
No reviewers
e
Labels
Clear labels   
blocker

blocks release until resolved

  
bug

   
code quality

   
confirmed

   
critical

crashes, catastrophic bugs

  
discussion

   
high priority

   
incompatibility

incompatible with mcl2

  
incomplete feature

   
invalid

   
low priority

   
missing feauture

   
needs testing

   
packet spam

   
performance

   
project

meta-issues

  
regression

   
translations

   
unconfirmed
  
in review

   
ready for review
No Label
blocker
bug
code quality
confirmed
critical
discussion
high priority
incompatibility
incomplete feature
invalid
low priority
missing feauture
needs testing
packet spam
performance
project
regression
translations
unconfirmed
in review
ready for review
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
2 Participants
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: Mineclonia/Mineclonia#72
No description provided.
Delete Branch "banner-mask-fix-3"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?