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

Image.replace_color #19234

Closed
wants to merge 1 commit into from

Conversation

AlexHolly
Copy link
Contributor

@AlexHolly AlexHolly commented May 29, 2018

I have to reopen an old discussion. #9623

I tried to use it with GDScript and my CPU goes wild, when I use this PR it stays calm.

Bugsquad edit: This closes #10646.

@akien-mga akien-mga added this to the 3.1 milestone May 29, 2018
@akien-mga akien-mga requested a review from reduz May 29, 2018 06:15
@akien-mga
Copy link
Member

akien-mga commented May 29, 2018

Well the arguments raised in #9623 still stand, I'm not sure such a pixel-art-specific method should be added to Image. Colors are floats so in most cases the comparison will return false, even if it's 99.999% the same color.

What's your use case? If it's for image edition (to save a modified image to disk) I can understand, but if it's just because you want to change some colors in-game, use a shader, it will be 100 times faster.

@akien-mga akien-mga changed the title Image.replace.color Image.replace_color May 29, 2018
@groud
Copy link
Member

groud commented May 29, 2018

I keep my point on view on that is still the same. That's too specific use case, which could be handled with a shader. Such image processing belong to a GDNative plugin.

@blurymind
Copy link

color indexing and palette swapping is such a huge part of gamedev though. The 8bit and 16 bit era games are built on it.

A lot of 2d indie games strive for that look and heavily rely on it :)
Not sure if this implementation is the best way to do it efficiently, but this has been requested a couple of times
#18269
#12670

@groud
Copy link
Member

groud commented May 29, 2018

Once again this can be done with a simple shader. Which is a way more pertinent way of handling color palettes.

@AlexHolly
Copy link
Contributor Author

I need it for my pixel tool I know a shader would be faster but that doesn't work in my case.

This is probably not the fastest way yes.. but works with 1000x1000 thats fine for now.

The fill function looks in the same category to me so why not. User who are new to gamedev have no clue about shaders they work with Image.

I tried GDNative a few months ago and ran into some bugs.

About the float topic not sure if this is the case. I transform my color to html I can use the color picker to change to any color I want (I know the previous color).

With this you can make different sets of sprites nice in pixel art (for beginners).

@groud
Copy link
Member

groud commented May 29, 2018

The question if whether such tool or not should be implemented in the core. For me the answer is no, since it's a corner case feature that will help only few people.

Since it needs performances, it should be implemented via GDNative.

@Zireael07
Copy link
Contributor

I am wondering, for my use case (recolor a map texture which is drawn pixel by pixel), how would one do it in shaders if this does not go in?

@Zylann
Copy link
Contributor

Zylann commented May 29, 2018

Colors are floats so in most cases the comparison will return false

Then a solution would be to not use get/set_color, which are slow, but convert the 2 colors into their data representation (1 to 4 bytes) with threshould rounding if needed (easier if we target only specific formats), and run replacement on that directly. It's way more efficient and won't suffer float precision issues if color arguments are picked correctly before the replacement loop. Such optimization would make sense only in C++ though, since GDScript has no language tools to do so.

If GDNative is too much of an undertaking for such a simple operation, a shader is more efficient at this and non-destructive. This is the modern way to do realtime color swappings.
But to be used in a painting program it needs good understanding of viewports, textures and of course shaders. It also cannot be a synchronous operation if done that way, because you need to wait 1 frame for the shader to take effect and get back your modified image.
@Zireael07 I made one, which I used to replace black outlines with red for a damage flickering effect, unfortunately I don't have access to it right now. Maybe later when I get back home. It basically checks the distance between the pixel color and the key color, and outputs the target color if under a threshold (which would be something like 0.01 if you need to replace 1 specific color).

Edit: here is a project showing a colorkey shader:
ColorKeyShader.zip

@AlexHolly
Copy link
Contributor Author

What is the difference between read and write? Why does write also work on the read operation.
https://github.com/godotengine/godot/blob/master/core/image.cpp#L846

@mhilbrunner
Copy link
Member

mhilbrunner commented Jun 7, 2018

@akien-mga From the previous discussion:

It seems indeed quite limited to low-res pixel art games

I don't think that is bad, as Godot is used a lot for such games, and one of the things that sets us apart from others is that we do support such games as much as big, 3D ones.

IMHO, the method name just needs to make clear that it won't work with anything but small, exact values.

@akien-mga
Copy link
Member

Well, if adding such a feature it should be as @Zylann described it, or as the magic wand in GIMP works: you give a base color and a threshold/precision, and it will replace all pixels within base_color ± threshold.

@mhilbrunner
Copy link
Member

Ah, yeah that would indeed be better.

@AlexHolly
Copy link
Contributor Author

Yeah adding threshold is a good addtition.

So simply something like float threshold=0.0 right? Or do you think a Vector4 makes sense too?

@mhilbrunner
Copy link
Member

Closing this, as there is no consensus on adding more Image editing methods to the Image class.

It may make sense to add Image.set_pixels(array) and Image.set_pixelsv(array) methods to speed this up when doing it from GDScript.

For this PR, a workable alternative may be to use GDNative. Still, thank you for your effort!

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.

Color replace modulation option
7 participants