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

Object properties remove colors #15685

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

cx384
Copy link
Contributor

@cx384 cx384 commented Jan 16, 2025

This PR removes the colors field from object properties.

It is not (sufficiently) documented, quite useless and takes up space.

This is the only place where it gets used, so only for upright_sprite with glow < 0, and the default color is white, but I didn't notice any difference, so I completely removed it instead of defaulting it to white., readded it, but I'm not sure if is needed.
https://github.com/minetest/minetest/blob/c8b5e3b741390fb98bfa9cdbae56930239ea271a/src/client/content_cao.cpp#L1406-L1408

The documentation for glow says Value < 0 disables light's effect on texture color. and this sill works.

To do

This PR is Ready for Review.

How to test

core.register_entity("testentities:upright_sprite2", {
	glow = -5,
	initial_properties = {
		visual = "upright_sprite",
		textures = {
			"testentities_upright_sprite1.png",
			"testentities_upright_sprite2.png",
		},
	},
})

@cx384 cx384 added Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements Code quality labels Jan 16, 2025
@cx384 cx384 force-pushed the object_remove_colors_field branch from d5b2a0e to 8dee4d9 Compare January 16, 2025 18:08
@cx384 cx384 mentioned this pull request Jan 16, 2025
3 tasks
@sfan5
Copy link
Collaborator

sfan5 commented Jan 17, 2025

I think hardware-coloring for entities isn't so far-fetched so I'd keep this property but just document clearly that it does nothing right now.

@appgurueu
Copy link
Contributor

As a minor side note, removing this property is technically a breaking change: Mods could be relying on :get_properties().colors existing, and they could expect to get back what they put in via :set_properties{colors = ...}.

This seems unlikely to me since this doesn't do anything right now, but you never know. Maybe someone is abusing this to store a couple colors (which is a really odd, bad way to store that, but 🤷).

@cx384
Copy link
Contributor Author

cx384 commented Jan 17, 2025

I think hardware-coloring for entities isn't so far-fetched so I'd keep this property but just document clearly that it does nothing right now.

Is this really something we want to add?
I thought it is not necessary, since you can just apply a texture modifier by changing the textures field.
Also, for object textures, I think it is likely that we will get something similar to the Tile definition of nodes (which includes hardware coloring).

The documentation for object properties says:

These properties are not persistent, but are applied automatically to the corresponding Lua entity using the given registration fields. Player properties need to be saved manually.

So I think it is clear enough that you can't use those to store your own data. There could always be someone abusing unused things, e.g. if you don't use a _ prefix for a custom field when registering a node.

@appgurueu
Copy link
Contributor

appgurueu commented Jan 18, 2025

Is this really something we want to add?

Maybe. The problem with how texture modifiers work is that it requires creating and processing a new texture. Hardware coloring could be more efficient since all you have to do is pass a bunch of colors to a shader.

Arguably we could probably implement hardware coloring as a special optimization for texture modifiers here, so something like "if it's simply some base texture with a [multiply modifier, then turn that into hardware coloring applied to the base texture instead".

So I think it is clear enough that you can't use those to store your own data.

Well, it seems to me that the way the docs are written, and the way the code behaves, we are guaranteeing that this property exists, that you can put colors in there and get them back out, same as for the other properties, while the entity is active (whether the entity is persisted or not; "Store" doesn't mean "store persistently" here).

Modders shouldn't be doing that, since for entities using a custom field, say self._colors, is strictly better, and for players you should use a table indexed by player name, but I've seen modders do stuff they shouldn't be doing but which works (for example storing data that should not be persisted in player meta because it's convenient).

Though as said, this is in my opinion very minor. I wasn't sure whether I should've brought it up at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code quality Maintenance Tasks to keep the codebase and related parts in order, including architectural improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants