CptSpaceToaster / CptsModdingLight

Do What The F*ck You Want To Public License
39 stars 15 forks source link

Dynamic Lights & Colored Lights Core #36

Open givemhell opened 9 years ago

givemhell commented 9 years ago

it seems that when you use Dynamic Lights mod with Colored Lights Core it does not render light from torches or other objects, granted it would be cool to see you support Colored Lights & movement but id just like to see the minor bug fixed =]

Link to Dynamic Light Mod - http://www.minecraftforum.net/forums/mapping-and-modding/minecraft-mods/1272478-dynamic-lights-portable-and-moving-lightsources

CptSpaceToaster commented 9 years ago

Most of the logic used inside DynamicLights strips out the color from the lightvalue integers, resulting in light that is rendered "black" by the CLC. I created an expectation for lightvalue integers, and it's not preserved in DynamicLights.

Last I recall, it also uses its own updateLightByType() method to spread light values around in specific instances to facilitate the dynamic movement. This method is also not compatible with CLC.

I can't fix this one on my end, unless they started relying on vanilla methods again................ or altered their source unnecessarily.

Lightvalue bitpacking S = Sunlight Bit B = Blue light Bit G = Green light Bit R = Red light Bit L = Vanilla "brightness" Bit

Vanilla:        0000 0000 SSSS 0000 0000 0000 LLLL 0000
CLC:            0000 0000 SSSS BBBB GGGG RRRR LLLL 0000
Dynamic Lights: 0000 0000 SSSS 0000 0000 0000 LLLL 0000

The CLC modifies the vanilla lighting engine to spread around these color bits, and also modifies the renderer to accept them. The ligthing engine is quite robust and hard to break, but in this case... if Dynamic Lights provides their own... then I can't do anything about it.

The renderer was modified to "Ignore" the L bits. Instead, a color mask is applied over the drawn vertexes using the B, G, and R bits. When dynamic lights does their own thing, the renderer will draw a block, ignore the L value that Dynamic Lights put there, and then it will draw "no color" because the R, G, and B bits were removed/never put there in the first place.

givemhell commented 9 years ago

im going to link this page to the other mod author and see if there is anything he can do about it thanks for the quick reply!

AtomicStryker commented 9 years ago

Ok so as far i remember Dynamic Lights uses ASM to override the following line in World.computeLightValue: int blockLight = block.getLightValue(xxx); -> int blockLight = DynamicLights.getLightValue(xxx). Dynamic Lights internally uses the same block.getLightValue(xxx); to get the vanilla value. I'm assuming you have overridden getLightValue(xxx) further "down" the pipeline. My source looks like this: https://code.google.com/p/atomicstrykers-minecraft-mods/source/browse/DynamicLights/src/main/java/atomicstryker/dynamiclights/client/DynamicLights.java#156

I assume i can fix this by either detecting color bits in the int or changing the light.getLightSource().getLightLevel() result to flip its bits into the vanilla int instead of overwriting it. Do you concur?

AtomicStryker commented 9 years ago

I have now checked out your repo and you completely replace computeLightValue in your ASM. It appears if this is to be fixed, it has to be done from your side.

givemhell commented 9 years ago

wow you both are extremely active and reply fast, thank you both for the support =] i hope this will be resolved soon!

CptSpaceToaster commented 9 years ago

@AtomicStryker If I understand it, then it looks as if I'd need to simply check to see if DynamicLights is loaded, then simply replace my calls to block.getLightValue() with your atomicstryker.dynamiclights.client.DynamicLights.getLightValue()

Sound right?

AtomicStryker commented 9 years ago

That is correct.

CptSpaceToaster commented 9 years ago

@AtomicStryker In both functions? Or only computeLightValue? The other being: updateLightByType() I'm not seeing how that fixes everything... But I can move forward with the integration regardless......

AtomicStryker commented 9 years ago

Well, i only hook into the one function, and its enough for Dynamic Lights to work. I can only speculate why you need 2, but since Dynamic Lights internally queries block.getLightValue(xxx); anyway, at worst you add some overhead.

CptSpaceToaster commented 9 years ago

I think I'm reflecting into it incorrectly... looking into this.

CptSpaceToaster commented 9 years ago

@AtomicStryker Ok... learned a lot... I'm actually referencing the method properly now.

And... something is still stomping on the color bits https://code.google.com/p/atomicstrykers-minecraft-mods/source/browse/DynamicLights/src/main/java/atomicstryker/dynamiclights/client/IDynamicLightSource.java#34

I don't know who's limiting to 15, but if that happens, then the color bits in the light value are lost.

LightValues are stored as

0BBBB 0GGGG 0RRRR 0LLLL
and they're being capped to 0b1111. A torch which is colored a bit yellow should normally return
01010 01101 01110 01110
(decimal: 341454) and a ceiling is being applied, and causing it to return decimal 15, meaning a torch is receiving a vanilla light value that is greater than intended...

Ideally, I should find the limit, and simply remove it for CLC purposes. If the limit still needs to exist outside of CLC, then applying

return lightValue & 0xF;
is a little safer as that would return the expected vanilla value of 14 even in the presence of CLC. Do you have any idea where this ceiling is coming from?

AtomicStryker commented 9 years ago

15? It's a design choice. Notch said "let there be light, in full integer increments of 0 to 15". The idea was probably to use an efficient data storage of 4 bits

AtomicStryker commented 9 years ago

Where exactly is your colored light value being broken? Dynamic Lights should only give you a generic value from 0 to 15, which you then use as brightness to make your RGB value from

CptSpaceToaster commented 9 years ago

You are correct from vanilla's perspective. The colored light core however, uses the vanilla light value and bit-crams values representing R, G, and B, into the lightvalue.

vanilla: 00000 00000 00000 0LLLL
clc:     0BBBB 0GGGG 0RRRR 0LLLL

So to keep vanilla happy, I've left 0LLLL in there to be safe, and can revert to a vanilla value at any time by bitwise and-ing the value with 0xF. The rewritten lighting engine however, spreads around the additional colors I've added. You'll see code segments such as

blue_light =  (lightValue >> 15) & 0xF; // gets blue light channel 
Which allow the engine to work with all 4 channels of light.

So... the mere fact that I'm receiving a limited "generic value" 0 to 15 is the reason this is a problem.... I was expecting an integer as high as 507375. (0b1111011110111101111)

When I make calls to atomicstryker.dynamiclights.client.DynamicLights.getLightValue() here: https://github.com/CptSpaceToaster/CptsModdingLight/blob/dev/src/main/java/coloredlightscore/src/helper/CLWorldHelper.java#L444

I get an "uncolored value" limited from 0 to 15. I'm not certain if the issue is that a line in your mod says "return 15 if the value is above it" or if there's a line in vanilla's entity classes that does the limiting.... but something's squashing the value, and it's causing this issue

I could do something hacky, and say "Hey, if I don't have color, then assume it's white" But I'm certain the people asking for this integration wouldn't be happy if a blue glowstone block was white only when the player held it. (Or lava not being orange, or redstone torches red, etc.) The very nature of this issue is that lights start out colored... then are converted to "black" colored light (that is... light with a positive LLLL value, but all of the color components are zero) somewhere... and I don't know enough about the system to know exactly where.

AtomicStryker commented 9 years ago

From what i have seen, you overwrite the entire computeLightValue. Now, at the call of this method and your custom implementation of it, you get your bit modified colored int back. My idea for compatibility was to call atomicstryker.dynamiclights.client.DynamicLights.getLightValue() at that point in time, convert the 0-15 int result to a 0-1 float, and use RGB brightness multiplication with your colored int to get a colored dynamic int. Which you then render.

CptSpaceToaster commented 9 years ago

computLightValue will look at the 6 available adjacent blocks, take the highest light value, and then subtract by one. Compute will basically return what the current block should be. This method will of course ask for saved light values in the world, or it will ask your mod if there is an entity there holding a light.

Instead of calling world.getSavedLightValue() which returns a light value with color bits... I moved to yours, which works ok for blocks, but burning entities, torches on the ground, players holding torches, all have this ceiling. Is there some entity code your calling to get the light value from a torch when a player holds it?

This is all I can find... And I'm not sure if that's somehow creating this's ceiling.

I would have loved to do what you suggest, but it's not feasible. If I receive a colored light value that has been stripped of its color, and arbitrarily rounded to 15 then I have no clue what it should be normally. I can't color a lightvalue in this scenario, because my code doesn't know what the source is. It's asking your code, which is getting it from Entity somehow, and along the way, the color data is squashed.

AtomicStryker commented 9 years ago

Ok i think you are approaching this wrong. Dynamic Lights overwrites the "lowest" light result in the pipeline, while you are trying to get it somewhere way higher. I'm looking at your source before you started DL compat pushes and from what i can tell your lowest light retrieval / computation is at https://github.com/CptSpaceToaster/CptsModdingLight/blob/38988fce7b9f59639dd041edb75ecf94f0b3fb66/src/main/java/coloredlightscore/src/helper/CLWorldHelper.java#L156. Now, to get DL compatibility i would put new code above this return, along the lines of

if (dynamicLights != null)
{
    float DLBrightness = getDynamicLight.invoke(xxx) / 15; // which calculates a brightness from 0 to 1
    float brightness = computeRGBBrightness(currentLight); // which takes all your rgb bits and calculates a combined brightness from 0 to 1
    if (brightness < DLBrightness)
    {
        currentLight = increaseBrightnessRGB(currentLight, DLBrightness-brightness);
    }
}
CptSpaceToaster commented 9 years ago

Hmm.... I don't have a method that takes in a float, and adds arbitrary color, nor do I see a way to create one. The light needs to already be colored by this point. My mod does not operate like yours in which I hooked computeLightValue to point to a map of lights and their color values. I went as low as the lightValue field in block. So here when I ask for a block's light value... it's already colored. I literally crammed color bits into the vanilla field. I hope that's clear.

  1. The "lowest" light level from my perspective is the Block.lightValue field.
  2. My mod works on the basis that Block.lightValue can be most integers in the range 0 to 507375.
  3. World.getSavedLightValue, and getDynamicLight.invoke(xxx) both rely on the field that ends up there.
  4. Some code somewhere down the getDynamicLight.invoke(xxx) path sees that the Block.lightValue is higher than 15, and caps it to 15, thus removing the color.

From what I understand, you're proposing a solution that can't work. The ceiling put in place coming from getDynamicLight.invoke(xxx) fundamentally ruins CLC's operation, and I have no method by which I can computeRGBBrightness() as you suggest.

Example: A torch should return 341454, and the lightValue for a torch will yield is 341454. In CLC, I ask for a Block's lightValue, and I could receive 341454 if the block happens to be a torch. Additionally, asking world.getSavedLightValue() will also yield the bit-packed lightValue that is saved in the world. However something down your end of things limits it. There isn't a point where I ask "What type of block is this?" and then say "Oh, its a torch, quick color it" So I can't work around it. Instead, I'd need getDynamicLight.invoke(xxx) to return an integer above 15.

There's still a misunderstanding of my mod's operation, and I hope I've made this a bit clearer. Your work is amazing, and I respect your knowledge and the time you've taken to try and help. If we can find this ceiling, we should be good...

AtomicStryker commented 9 years ago

But that is my point! You do not overwrite your colored int with my 0-15 value. You only USE it to change the luminance of your colored int. Wait, isn't this https://github.com/CptSpaceToaster/CptsModdingLight/blob/38988fce7b9f59639dd041edb75ecf94f0b3fb66/src/main/java/coloredlightscore/src/helper/CLWorldHelper.java#L126 the luminance even? Just alter it to

int ll = dynamicLights == null ? neighborLight & 0x0000F : getDynamicLights.invoke(xxx);

Also to get brightness from rgb: https://stackoverflow.com/questions/596216/formula-to-determine-brightness-of-rgb-color In the reverse, to increase rgb brightness, you simply multiply your increase factor with 255, add it to each rgb and clamp to 255

CptSpaceToaster commented 9 years ago

You do not overwrite your colored int with my 0-15 value.

You are correct, I made a mistake, and replaced the wrong lookup :< I'll update source, link to the code. Then we STILL have this issue, but it will be more apparent

I replaced the wrong lookup: https://github.com/CptSpaceToaster/CptsModdingLight/issues/36#issuecomment-75171980

CptSpaceToaster commented 9 years ago

Here's where I should have hooked in your method to start: https://github.com/CptSpaceToaster/CptsModdingLight/blob/dev/src/main/java/coloredlightscore/src/helper/CLWorldHelper.java#L104

What you're kinda suggesting: To "add color" to the lightvalue. can occur as it should on line 106, but that is not where I make a blue light look blue. That correction only serves to say "Hey, this light has no color what so ever. So lets assume it was supposed to be white"

  1. If a block is blue, it will be colored blue no problem, because block.getLightValue returns 0b01111000000000001111
  2. If a block has no color, then it will be colored white (bug fix for "black colored lights" from other mods) 0b00000000000000000110 -> 0b00110001100011000110 - dim grey light, line 106
  3. If a player holds a blue light, then we still have our bug, because your end would simply say that it's returning 15... and then I would color it white because I have no idea what the original color was supposed to be.

Eh? Am I making any sense now?

AtomicStryker commented 9 years ago

I understand. Whether or not you want to make all dynamic lights white or not (i would suggest doing so) is your decision. This was about making our mods compatible.

CptSpaceToaster commented 9 years ago

So, I initially wanted to spread colored light around if a player was holding a colored light source...

Second, there's still a bug here, as I'm getting unexpected values from your end. (Either due to rounding issues, or altered values.

Oh... was it intentional for a held piece of glowstone to emit light level 12?

I thought you would pull from the block.lightValue directly! If you were, we'd have the ultimate dream of a player walking around with blue glowstone, lighting up the area blue! If you intend to alter the values like you are (redstone -> 10, glowstone -> 12) then I can respect it, but it leaves us off for the worse.

givemhell commented 9 years ago

why not make it go both ways and let the player decide on what he/she would pref through one of the configs?

Settings 1 Give dynamic lights priority for custom light ranges without color 2 Give colored lights priority for colored light spread from item without custom light range

or by default have it give dynamic lights & put out a patch/build that would allow colored lights priority

i could understand why you guys wouldnt want todo this, coding is a pain in the ass & why i gave it up in highschool XD

but i do love the idea of walking around with colored lights,

AtomicStryker commented 9 years ago

Dynamic Lights doesn't use the lightValue off blocks, correct. It was only ever configurable. Of course, you could implement your own module of Dynamic Lights and just turn my modules off ... in retrospect that solution might have been easier than all of this.

CptSpaceToaster commented 9 years ago

Alrighty... so it's either 1.) Learn a lot about your setup, hook into some sort of API/module system and disable everything (I presume I would then send through the default value myself in their place) 2.) Ask you to create a config option to permit Block.lightvalue through instead of the remapped values...

Looks like one of us is going to have to do some work here. I'd be willing to turn a wrench if you really don't want to open up a config option up in your mod, but I think I'd need a couple pointers on what I'm even looking to do with your module system.

AtomicStryker commented 9 years ago

Hmm i guess i could, maybe. But the thing is, your int values as such do not indicate which one is "strongest". I cant simply compare if (a < b), because a and b might be more or less random integers as a result of your bit flipping. I still think white light from dynamic light sources is a passable compromise.

CptSpaceToaster commented 9 years ago

Ooop! Not quite. I've thought through this one, as I needed to compare one light source against another. If LightValueA has any components greater than LightValueB, then I'd do the following:

if ((((0x100000 | LightValueB) - LightValueA) & 0x84210) > 0) {
    //frob widgets because some color components of A > B
}

Bitwise hackery! Basically, we subtract the two light values, and then look to see if there's anything in the carry-over bits. (the zero in between each light value)

  B: 01010 00000 00000 01010 - Blue Light
- A: 00000 00000 00011 00011 - Dim Red Light
-----------------------------------------
     01001 11111 11101 00111
           ^     ^
Carry over bits detected, that means that A has at least one color component greater than B

So that address THAT concern... Alternatively, you can part it out into 4 temp variables, check each one individually, and move on.

int temp_vanilla_brightness = (lightValue) & 0xF
int temp_red = (lightValue >> 5) & 0xF
int temp_grn = (lightValue >> 10) & 0xF
int temp_blu = (lightValue >> 15) & 0xF

This might also be useful, which is the last step in reverse.

CptSpaceToaster commented 9 years ago

I'm more than willing to fill out the API if you need this sort of functionality more readily available. The API is simply a collection of methods that work with the lighting-words. You can include it safely without forcing a hard-dep, but if it's not helpful, then I'd like to know :)

http://coloredlightscore.us.to/maven/clc/coloredlightscore/ColoredLightsCore/

AtomicStryker commented 9 years ago

https://code.google.com/p/atomicstrykers-minecraft-mods/source/detail?r=cdd3e5ff65bfb6eb1cec46c9d6a37cde9fbc1409

CptSpaceToaster commented 9 years ago

Tyvm: Quick comment: Line 60 of ItemConfigHelper

I think a call to block.getLightValue(IBlockAccess world, int x, int y, int z) would be a more reliable... but you don't have coordinates available. The CLBlock interface shows how one might color their own lightvalue based on metadata, but I don't expect all blocks to implement CLBlock. The interface is a suggestion/helper, and not a mandatory inclusion.

CLBlock simply wraps block.getLightValue(IBlockAccess world, int x, int y, int z)... so another mod may simply do that, and you wouldn't be able to hook... You can pull from b.getLightValue, but if the lightValue changes by the block's metadata (and someone didn't implement CLBlock), then we're out of options.

It works for now, but to get this good and proper... would you need a method in world, getLightValueByMeta(int meta) or something?

Thank you very much!

AtomicStryker commented 9 years ago

Ah, but where do i get xyz coordinates of a block which is not necessarily present anywhere in the world

CptSpaceToaster commented 9 years ago

Exactly... there isn't a good solution, because vanilla doesn't support different instances of LightValue per metadata... I simply hacked that in.

You can't cache the results from block.getLightValue(IBlockAccess world, int x, int y, int z) when you parse your config... so this is honestly the best solution... even if I asm'ed in a block.getLightValueByMeta() method... noone know would know how to use it unless I force them (same issue with implementing CLBlock)

Considering CLBlock is an abstract... that makes it even more invasive... I might have to change CLBlock to an interface that just tells you to implement the method you use. Force everyone to use that interface... and then... move the little jig to block.getLightValue(IBlockAccess world, int x, int y, int z) to another abstract class "EasyColoredLight" or something.

Also... I see what you did when the entity-light exists in the same location as a block... there's going to be an outstanding issue on that till I can fix my lighting engine...

If I read it correctly, if I stand in a yellow torch while holding a blue light, I will temporarily replace the torch's yellow light with my blue light (because the check sees that the blue light has components that the torch doesn't) What you need, is to "add both lights together" so the torchlight and blue light are grabbed together (and it just grabs the brightest color component of each for a bit)

public static int combine(int a, int b) {
    int a_lgh = a & 0x0000F;
    int a_red = a & 0x001E0;
    int a_grn = a & 0x03C00;
    int a_blu = a & 0x78000;

    int b_lgh = b & 0x0000F;
    int b_red = b & 0x001E0;
    int b_grn = b & 0x03C00;
    int b_blu = b & 0x78000;

    return ( Math.max(a_blu, b_blu) | Math.max(a_grn, b_grn) |
             Math.max(a_red, b_red) | Math.max(a_lgh, b_lgh) );

So I'll add that to the API... that's easy for me to do, and who knows what my lighting engine will do once you patch it in...

Consider it a non-issue till I can fix https://github.com/CptSpaceToaster/CptsModdingLight/issues/39

tyvm AtomicStryker! You've been amazing, and I really appreciate everything you've done.

givemhell commented 9 years ago

same here both of you offer the best light mods out there, im really glad to see you guys work this out.