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 node texture variants #13811

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add node texture variants #13811

wants to merge 5 commits into from

Conversation

sfence
Copy link
Contributor

@sfence sfence commented Sep 15, 2023

This is the adoption of #12928. Closes #9193

I have rebased the original @TurkeyMcMac code and tested it.
It looks to be working without some artifact on my Linux.

To do

This PR is Ready for Review.

How to test

There are test nodes in the developer test game. They can be used to test.
There is also the command /node_visual which allows to change variant_offset for the wanted node.
Example of command: /node_visual testnodes:variant_color variant_offset 2

@sfence
Copy link
Contributor Author

sfence commented Sep 15, 2023

According to this: https://irc.minetest.net/minetest-dev/2023-01-08#i_6046142

it seems that there is nothing left to be done.

Am I right?

@wsor4035 wsor4035 added @ Server / Client / Env. Feature ✨ PRs that add or enhance a feature Textures labels Sep 15, 2023
@rubenwardy rubenwardy added the Concept approved Approved by a core dev: PRs welcomed! label Sep 17, 2023
@Bituvo
Copy link
Contributor

Bituvo commented Nov 21, 2023

Would it be possible to add weights for each variant?

@sfence
Copy link
Contributor Author

sfence commented Nov 30, 2023

Would it be possible to add weights for each variant?

What do you mean by it?

@Bituvo
Copy link
Contributor

Bituvo commented Dec 1, 2023

What do you mean by it?

So that one texture is more likely to appear than another, for example. Like, dirt_1.png could have a 75% chance of appearing, but dirt_2.png could have a 25% chance.

@sfence
Copy link
Contributor Author

sfence commented Dec 1, 2023

What do you mean by it?

So that one texture is more likely to appear than another, for example. Like, dirt_1.png could have a 75% chance of appearing, but dirt_2.png could have a 25% chance.

So, I understand now.

This uses the node param2 value to choose the texture. So if the mod author wants to make some texture more common, he only needs to reflect in on_construct callback or/and in the mapgen generation code.

doc/lua_api.md Outdated Show resolved Hide resolved
doc/lua_api.md Outdated Show resolved Hide resolved
Copy link
Contributor

@doxygen-spammer doxygen-spammer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here Github frustrates me again, as it refuses to expand the C++ diffs. :(

I left comments in the code.

Maybe it should be made clear if you need to override the same set of properties in each variant, or if it is allowed to e. g. add an overlay tile only to variants 3 and 6.

@sfence
Copy link
Contributor Author

sfence commented Dec 21, 2023

Here Github frustrates me again, as it refuses to expand the C++ diffs. :(

I left comments in the code.

Maybe it should be made clear if you need to override the same set of properties in each variant, or if it is allowed to e. g. add an overlay tile only to variants 3 and 6.

So, It should be documented now as well.

There is probably a bug in overlay_tiles which uses color. I will look at it later.

@doxygen-spammer
Copy link
Contributor

doxygen-spammer commented Dec 24, 2023 via email

@sfence
Copy link
Contributor Author

sfence commented Dec 29, 2023

With the last commit, this PR should be able to fix issue #13417 as well and possibly make issue #13112 obsolete.

@doxygen-spammer
Copy link
Contributor

doxygen-spammer commented Dec 29, 2023 via email

@doxygen-spammer
Copy link
Contributor

doxygen-spammer commented Dec 29, 2023 via email

@sfence
Copy link
Contributor Author

sfence commented Dec 30, 2023

@doxygen-spammer
I have rebased this. It includes lua_doc.md update, so the set/get_node_visual methods are documented now.

@doxygen-spammer
Copy link
Contributor

doxygen-spammer commented Dec 30, 2023 via email

@kromka-chleba
Copy link
Contributor

With the last commit, this PR should be able to fix issue #13417 as well and possibly make issue #13112 obsolete.

It would indeed fix the first issue but not the second one. ABMs and LBMs do more stuff than simply swapping textures. Sometimes you want to remove the node completely, move the nodes or maybe spawn a structure. Not being able to change ABMs/LBMs after they are created makes them inflexible while hitting performance.

@Zughy Zughy added Action / change needed Code still needs changes (PR) / more information requested (Issues) Rebase needed The PR needs to be rebased by its author. and removed Action / change needed Code still needs changes (PR) / more information requested (Issues) labels Jan 21, 2024
@Zughy
Copy link
Member

Zughy commented Jan 21, 2024

@sfence rebase needed

@Zughy Zughy removed the Rebase needed The PR needs to be rebased by its author. label Jan 22, 2024
@sfence sfence force-pushed the variant branch 2 times, most recently from 57936a5 to 330536b Compare January 25, 2024 18:09
@Zughy
Copy link
Member

Zughy commented Mar 24, 2024

(just in case: rebase still needed, one file conflicting)

@sfence
Copy link
Contributor Author

sfence commented Mar 24, 2024

@Zughy Yeah... some conflict PR has been merged during rebasing... :D

@sfence sfence mentioned this pull request Apr 4, 2024
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Concept approved Approved by a core dev: PRs welcomed! Feature ✨ PRs that add or enhance a feature @ Server / Client / Env. Textures
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add paramtype2 for node texture variants
8 participants