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

MapInfo/ZScript Lightmap Control #71

Merged
merged 6 commits into from
Oct 24, 2024

Conversation

MrRaveYard
Copy link
Collaborator

@MrRaveYard MrRaveYard commented May 6, 2024

Proof of concept.

Demonstration for one possible use:
https://www.youtube.com/watch?v=9_Utp2CLC9E

It still has some bugs and certain things I haven't given much thought:

  • should sun direction be towards or away from sun?
  • futureproof a multiple sun support?
  • move zscript functions into levellocals or keep as it is?
  • UI scope? Play scope? Data scope?

etc.

MapInfo:

GameInfo
{
	forceEnableLightmaps = true  // to enable lightmaps in all levels (does not regenerate existing lightmaps)
	defaultSunColor = 1.0, 1.0, 1.0
	defaultSunDirection = 0.45, 0.3, 0.9 
}

ZScript:

Lightmap.SetSunColor((1.0, 1.0, 1.0)); // vector3 rather than color, to support extra brightness
Lightmap.SetSunDirection((0.4, 0.2, 1.0)); // converted into unit vector inside the native function
Lightmap.Invalidate(); // same as `invalidatelightmap` ccmd, but also invalidates actor trace lights cache

@Boondorl
Copy link
Contributor

Boondorl commented Aug 13, 2024

To comment on the scoping, I would make the setters data scope but if there are any getters, they should be UI scoped. The playsim shouldn't have access to reading any rendering properties, otherwise it then has to be considered synchronized and networkable which is bad for gameplay implications. I would also generally keep things in LevelLocals that are meant to modify the current level as the lightmap is set on a per-map basis rather than generically like a post processing shader (which is ideally when it'd get its own namespace).

For direction, I would make it the direction the sun is facing since it doesn't really make a ton of sense the other way. I might also consider making the direction setter take yaw and pitch instead for ease of user understanding (I wouldn't worry about efficiency here since spamming this functionality is going to cause problems regardless), and sun color should be broken into three separate arguments rather than a vec3 imo.

I'm iffy if invalidating is something that should be allowed manually but I don't know the exacts of why it's needed. If it's something that must always be done after using these setters, it shouldn't be exposed and should instead be baked into the setters. Making a function that changes both direction and color at the same time would be more practical if the idea is efficiency, and if a user is making a ton of changes at once, they should be preprocessing that info rather than constantly calling the setters. Both methods have their pains, but one adds more boilerplating than the other for a mandatory step that I'm assuming can't be skipped.

@MrRaveYard
Copy link
Collaborator Author

To comment on the scoping, I would make the setters data scope but if there are any getters, they should be UI scoped. The playsim shouldn't have access to reading any rendering properties, otherwise it then has to be considered synchronized and networkable which is bad for gameplay implications.

Makes sense.

I would also generally keep things in LevelLocals that are meant to modify the current level as the lightmap is set on a per-map basis rather than generically like a post processing shader (which is ideally when it'd get its own namespace).

I do really like the lightmap namespace, but I understand why it should be under level. I'll see what I can do.

For direction, I would make it the direction the sun is facing since it doesn't really make a ton of sense the other way.

Right, I'll change that to match ZDRayInfo thing.

I might also consider making the direction setter take yaw and pitch instead for ease of user understanding (I wouldn't worry about efficiency here since spamming this functionality is going to cause problems regardless),

I'll probably put that into the ZScript side as a wrapper function.

and sun color should be broken into three separate arguments rather than a vec3 imo.

Personally, I find it more convenient to be able to pass a vector as is, instead than having to write out x, y, z.

I'm iffy if invalidating is something that should be allowed manually but I don't know the exacts of why it's needed. If it's something that must always be done after using these setters, it shouldn't be exposed and should instead be baked into the setters.

Lightmapping an entire level is expensive. If someone uses lm_dynamic surfaces or implements feature allowing you to invalidate only specific portions of the level, you'd have to make an alternative for this function or introduce flags to say "don't rebuild the entire lightmap please".

bool forceEnableLightmaps = false;
FVector3 defaultSunColor = FVector3(1.f, 1.f, 1.f);
FVector3 defaultSunDirection = FVector3(0.45f, 0.3f, 0.9f);
int defaultLightmapSampleDistance = 16;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apologies for the merge conflicts 🙏 We're using a default value of 8 now, to match UDB's default, plus 16 is a little too blurry to be usable...

@nashmuhandes nashmuhandes marked this pull request as ready for review October 24, 2024 03:11
@nashmuhandes nashmuhandes merged commit e86b092 into dpjudas:master Oct 24, 2024
0 of 8 checks passed
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.

3 participants