jMonkeyEngine / jmonkeyengine

A complete 3-D game development suite written in Java.
http://jmonkeyengine.org
BSD 3-Clause "New" or "Revised" License
3.74k stars 1.12k forks source link

Bright and dark (ColorRGBA - Functions) #2240

Closed JNightRide closed 2 months ago

JNightRide commented 2 months ago

Hello everyone, this PR is about adding two new methods to the ColorRGBA class where you can make colors brighter or darker.

The difference between method brighter() and get|setAsSrgb()is the following:

Also add an example class (ColorBrighterTest) so you can try it out.

[ Some screenshots ]

Captura desde 2024-04-06 19-18-57 Captura desde 2024-04-06 19-19-10

What do you think of this idea?

pspeed42 commented 2 months ago

darker() seems no different than mult() with a fixed factor... so less flexible. What not just call mult(0.7)?

brighter() is not that different from mult() either... except again with a fixed factor. Other than the 0 handling, this is just another mult() call.

The factor seems rather arbitrary. Why not a parameter? Using a constant doesn't really make any sense.

Also, neither of these are at all compatible with srgb color because they presume that colors are already in linear space. (The fact that you compare brighter() to setSrgb() makes me think you might not understand what setAsSrgb() is really doing... hint: it is NOT just making the color brighter.) I only bring up srgb because you did, though.

These new methods seem unnecessary to me. They do one thing and they do it rigidly. A new multRgb(float) that skips alpha and a new minLocal() method would take the place of all of this and have way more uses.

JNightRide commented 2 months ago

darker() seems no different than mult() with a fixed factor... so less flexible. What not just call mult(0.7)?

This is so that the user has a fixed value for the dark color (the intention is not for it to be dynamic in these cases, there are the methods you mentioned).

Also, neither of these are at all compatible with srgb color because they presume that colors are already in linear space. (The fact that you compare brighter() to setSrgb() makes me think you might not understand what setAsSrgb() is really doing... hint: it is NOT just making the color brighter.) I only bring up srgb because you did, though

Mention method 'setSrgb()' to give an example (not that these new methods are replacing them or anything, not that they are related either)

brighter() is not that different from mult() either... except again with a fixed factor. Other than the 0 handling, this is just another mult() call.

comparing it to 'mult()' is an error; 'mult()' simply multiplies the values ​​of 2 colors, 'brighter()' increases the clarity of a color (brightness) in a given range so that the user does not have to intervene

These new methods seem unnecessary to me. They do one thing and they do it rigidly. A new multRgb(float) that skips alpha and a new minLocal() method would take the place of all of this and have way more uses.

As mentioned above, the intention is not to energize it (user intervention); the methods mentioned would not be bad (minLocal() , multRgb(float) -> names)...

JNightRide commented 2 months ago

I'll close this PR because it's "redundant".