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

Directional light #1554

Closed
wants to merge 13 commits into from
Closed

Conversation

nashmuhandes
Copy link
Contributor

@nashmuhandes nashmuhandes commented Feb 19, 2022

Adds an infinite distance directional light. Its main purpose is to light models (and optionally walls and flats) over a large space without spamming many dynamic lights, or creating a large dynamic light with a ridiculous radius size (and therefore lead to performance degradation).

Also helps with bringing out materials without spamming dynamic lights everywhere.

It is opt in via new MAPINFO keywords.

DirectionalLightMode = mode
0: disabled (default)
1: directional light on models only
2: directional light on models + the world

DirectionalLight = X, Y, Z, strength
Determines the XYZ vector of the directional light, and the strength.

New ZScript functions:

// Sets the directional light
native void SetDirectionalLight(double x, double y, double z, double strength);

// Retrieve current directional light
native Vector3 GetDirectionalLightVector(void);

// Retrieve current directional light strength
native double GetDirectionalLightStrength(void);

Demo maps attached. Due to the nature of directional lights, it is recommended to use the EvenLighting model on maps that utilize this feature.

Example 1: Just a simple map showing off the directional light bringing out the material on the sludge, as well as the traffic barriers.
DirLightTest01.zip

Example 2: 4 different maps selectable to demonstrate the features.
DirLightTest02.zip

I have tested this thoroughly and it is confirmed to be working for the hardware OpenGL and Vulkan backends. OpenGLES hasn't been touched as I am unfamiliar with that tech.

@MajorCooke
Copy link
Contributor

I have been waiting for this... for so long. I hope it's merged in soon.

Copy link
Member

@coelckers coelckers left a comment

Choose a reason for hiding this comment

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

Some issues here:
The check for walls is in the wrong place. It must be outside he WALLF_EXTCOLOR check.
If you provide a script interface it should be able to change the mode, too, not just the vector. But there's neither a function nor is the variable writable.

Also, for shader optimization it may be preferable to put the directional light vector into the static viewpoint uniform buffer and only pass a single flag with the stream data that enables it.
uTextureMode already contains a few flags, so it could be added here without increasing the dynamic buffer data at all.

@inkoalawetrust
Copy link
Contributor

Adding on what Graf said about also making the lighting mode changeable. I think it would also be a good idea to make ACS function equivalents for this new feature. Since modifying how the directional lighting on the level behaves is also directly relevant to what ACS exists for.

@nashmuhandes
Copy link
Contributor Author

Adding on what Graf said about also making the lighting mode changeable. I think it would also be a good idea to make ACS function equivalents for this new feature. Since modifying how the directional lighting on the level behaves is also directly relevant to what ACS exists for.

There will be no additions to ACS. Users can use the ScriptCall function to interface their ACS with ZScript.

As for everything Graf requested - acknowledged, will get to them as soon as I can. Currently occupied with some other tasks.

@inkoalawetrust
Copy link
Contributor

inkoalawetrust commented Feb 21, 2022

There will be no additions to ACS. Users can use the ScriptCall function to interface their ACS with ZScript.

Why not ? This is literally the point of ACS, to make scripted and dynamic maps. And this is a feature that pertains to maps themselves a lot. So it would be pretty topical and relevant to ACS' purpose to have a few functions to mess with the directional lighting on a map. Instead of having to make several unnecessary static wrapper functions and a class in ZScript.

@MajorCooke
Copy link
Contributor

MajorCooke commented Feb 22, 2022

Because ZScript exists and you have ScriptCall now. I for one am never doing any additions to ACS ever again for my stuff.

@inkoalawetrust
Copy link
Contributor

Because ZScript exists and you have ScriptCall now. I for one am never doing any additions to ACS ever again for my stuff.

So what, ACS is deprecated too now ?

@MajorCooke
Copy link
Contributor

MajorCooke commented Feb 22, 2022

Too much effort for too little gain. But yeah, I imagine it is considered so, or at the very least no one wants to develop for it anymore.

Creating a quick wrapper is easy enough for everyone. Manually adding ACS coding specifically for this on the other hand would require a LOT more programming.

@inkoalawetrust
Copy link
Contributor

Too much effort for too little gain. But yeah, I imagine it is considered so, or at the very least no one wants to develop for it anymore.

Yeah, it's considered deprecated for modders, not mappers. Setting up stuff like delays is also not as simple in ZScript as ACS.

Creating a quick wrapper is easy enough for everyone. Manually adding ACS coding specifically for this on the other hand would require a LOT more programming.

How would a few ACS functions to get different variables out of a vector, and to set them back again require a lot more programming ?

@Talon1024
Copy link
Contributor

Too much effort for too little gain. But yeah, I imagine it is considered so, or at the very least no one wants to develop for it anymore.

Yeah, it's considered deprecated for modders, not mappers. Setting up stuff like delays is also not as simple in ZScript as ACS.

I agree. It's not like there's a ZScript_Execute line special or a Delay function in ZScript. I suppose the closest one could come to ACS-like scripting in ZScript would be an abstract ScriptedEvent class wherein mappers queue up line specials and other actions to perform.

Creating a quick wrapper is easy enough for everyone. Manually adding ACS coding specifically for this on the other hand would require a LOT more programming.

How would a few ACS functions to get different variables out of a vector, and to set them back again require a lot more programming ?

Adding the functions to ACS would require additions to ACC and its libraries, as well as registering and implementing the functions in p_acs.cpp. With a static function in ZScript, it would require a simple ScriptCall, and no additions to ACC.

// ACS without ScriptCall example
script 1724 (void) {
    SetDirectionalLight(1.0, 1.0, -0.25, 1.0);

    int dlx = GetDirectionalLightX();
    int dly = GetDirectionalLightY();
    int dlz = GetDirectionalLightZ();
    int dls = GetDirectionalLightStrength();
}

// ACS + ScriptCall example
script 1724 (void) {
    ScriptCall("ACSTools", "SetLightDirection", 1.0, 1.0, -0.25);

    int dlx = ScriptCall("ACSTools", "GetLightDirectionX");
    int dly = ScriptCall("ACSTools", "GetLightDirectionY");
    int dlz = ScriptCall("ACSTools", "GetLightDirectionZ");
    int dls = ScriptCall("ACSTools", "GetLightStrength");
}

// ZScript class
class ACSTools {
    static void SetLightDirection(double x, double y, double z) {
        double strength = LevelLocals.GetDirectionalLightStrength();
        Vector3 direction = Vector3(x, y, z);
        LevelLocals.SetDirectionalLight(direction, strength);
    }
    static double GetLightDirectionX() {
        Vector3 direction = LevelLocals.GetDirectionalLightVector();
        return direction.x;
    }
    // You should get the idea by now
    ...
}

BTW, is this a new iteration of #1133 ?

MajorCooke added a commit to ZDoom/qzdoom that referenced this pull request May 21, 2022
@nashmuhandes nashmuhandes force-pushed the DirectionalLight2022 branch 2 times, most recently from 563c77b to 3f652b0 Compare May 31, 2022 16:25
@madame-rachelle
Copy link
Collaborator

madame-rachelle commented Jan 21, 2023

This PR is getting a little stale and has merge conflicts - I'm going to close this one for now, a new one can be made after the problems have been addressed for a fresh review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants