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

Shuttle & Predator ship lighting fixes #5221

Closed
wants to merge 6 commits into from
Closed

Shuttle & Predator ship lighting fixes #5221

wants to merge 6 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Dec 15, 2023

About the pull request

This PR contains the following changes

  • Corrected visual issues on tiles on predator ship
  • Corrected floorlight issues after turning off due to overlays being cut and not reapplied in open turfs update_icon function
  • Corrected shuttle ships lighting
  • Fixed where shuttle lights turned off after taking on, but not back on, as well as the shuttle light could never be shut off
  • Added the marine_dropship/on_arrival callback, so that dropship equipment have the expected event triggered. This event was not triggering for the spotlights because it was missing.

Explain why it's good for the game

This PR fixes various lighting related issues which are explained above as well as better explained in the images below. Overall these changes bring a better lighting experience for both marines and predators.

Testing Photographs and Procedure

Important notes

It is important to note that the shuttle only appears graphically correct when the base alpha is above 128. I tried playing with the colour matrix and other methods to have them consistent in all situations, but I am unsure if this is even a use-case which happens. Please let me know if there is a need to handle this situation and I can spend more time on it, but the alpha masking changes only apply to the shuttles using the use_alphamasking property.

Additionally, these changes add a method which should be used to get lighting effects for base lighting instead of referencing the property directly from the area. It is named get_turf_lighting_effect

Additionally, while I have not been able to spot any graphical issues, the changes to the lighting_effect property do affect the base lighting for all tiles, so any feedback on things to look out for would be welcomed. I have checked the ship over and common areas on surface maps but it can be tricky to spot things.

Screenshots & Videos

Tile colour blending changes

Before

colour_blending_before

After

colour_blending_after

Floorlight fix

Before

light_tile_offstate_before

After

light_tile_offstate_after

Dropship lighting changes

Before

shuttle_lightsoff_before
shuttle_lighting_surface_lightsoff_before

After

dropship_lighting_after
shuttle_lighting_surface_lightsoff_after

Changelog

🆑
fix: Fixed marine shuttle and predator ship lighting issues
fix: Fixed floorlight lighting issues
fix: Fixed marine shuttle spotlight toggling issues
/:cl:

@github-actions github-actions bot added the Fix Fix one bug, make ten more label Dec 15, 2023
@ghost ghost marked this pull request as ready for review December 15, 2023 18:33
@cuberound
Copy link
Contributor

could you possibly add those floor ligts that are used on.. BR LZ I think? might help with the hangar lighting

@ghost
Copy link
Author

ghost commented Dec 16, 2023

could you possibly add those floor ligts that are used on.. BR LZ I think? might help with the hangar lighting

This PR focuses around code changes to improve the lighting and would be out of scope. I'm not sure who would be best to talk to about mapping changes but I am just looking to improve the lighting code.

Copy link
Contributor

This PR has been inactive for long enough to be automatically marked as stale. This means it is at risk of being auto closed in ~ 7 days, please address any outstanding review items and ensure your PR is finished, if these are all true and you are auto-staled anyway, you need to actively ask maintainers if your PR will be merged. Once you have done any of the previous actions then you should request a maintainer remove the stale label on your PR, to reset the stale timer. If you feel no maintainer will respond in that time, you may wish to close this PR youself, while you seek maintainer comment, as you will then be able to reopen the PR yourself

@github-actions github-actions bot added the Stale beg a maintainer to review your PR label Dec 24, 2023
@ghost
Copy link
Author

ghost commented Dec 24, 2023

@fira

In response to the questions on discord:

98d6b07bc34025169cf3caaddda20c88

I have a few things to say code wise

Assuming the outcome this PR is working to achieve is desired, I am happy to make any code changes you see fit. This is my first PR outside of minor code changes and want to adhere to the project standards.

Why do we need to seperately multiply mask contents?

The base lighting area is effectively 1.0, so if we are to add the darkness together we will get something like (1.0+0.2) = 1.2 (clamped to 1.0)
When multiplying we will overcome this and have something like (0.2*1.0)=0.2. But this explanation is a bit vague and while I could just say "it works" and point to the screenshots, that is not really a great explanation, so I will try to point to other resources online to explain it.

While these references are not specific to byond, the blending principles remain the same.

It is complicated to explain and also I am not exactly a lighting expert, however based upon what resources I can find as well as my observation in game, this is the desired effect here, but even so this is limited to only the effected turfs in the shuttle area. Basically in this situation we are trying to darken the turfs, not brighten them. Currently as shown in the screenshots they are maximum brightness due to the base alpha settings. If we are to remove those settings then it does look correct, but the entire interior of the shuttle is dark. We could actually just solve the problem by having light sources in the shuttle as well.

In short, since we don't want to remove the base alpha property from the shuttle (or else we would need light sources added and I believe its desired to be bright at all times), we must remove light from the turfs in question rather than add light to them (they are already maximum brightness)

Update to above question

Upon re-reading this I believe you mean the alpha masking with the get_alphamask_apperance function. The reason for this is because we don't want the items ontop of the turf to be dark, just the turf itself. This applies similar properties but only for the icon on top (for example the corner pieces of the shuttle), all of which are already marked as transparent. Maybe a comment should be added that this should only be used for indestructible contents, however I will wait for feedback before making more changes to this PR.

The below pictures better illustrate why the get_alphamask_apperance was added:

Without

without_alphamask_appearance

With

with_alphamask_appearance

Why is this a turf dependent and area wide setting

In the shuttle (area), the turf type selected is the only one where this lighting issue is visible. I did not think it would be wise to change the lighting properties for all areas when the shuttle seems to be a unique case for this problem. If more areas exist or it is desirable to do this for all areas then such a change can be made, but I am unaware of anywhere else at this point, and opted to apply the lighting changes to only the affected area, allowing people to tweak this setting of other areas come up in the future.

Why only for the shuttles:

This same issue can occur anywhere base lighting happens beside non-base lighting areas, however I am not aware of many places where this occurs other than some admin areas, so I only enabled these settings for the shuttle.

What if contents change

I will assume you mean the area contents and not shuttle, because these changes do not affect the interior of the shuttle.
If the contents change than the person changing them will need to consider the lighting properties, and at the time adjust the settings to be appropriate to look graphically correct for those changes. In terms of the alpha masking of the turf contents, this indeed should only be used in areas with few contents such as the shuttle. If someone were to deconstruct those pieces of the shuttle and build something new, it would be dark instead of bright, however I believe the shuttle is completely indestructible.

Closing

I hope these answers are sufficient and satisfy what you are asking, I do not mean to just blast you with a pile of links but it is a little difficult to explain, however based on various readings as well as the observed results from these code changes I believe it is correct to solve the lighting issues shown in the "before" screenshots on the shuttle.

@github-actions github-actions bot removed the Stale beg a maintainer to review your PR label Dec 25, 2023
@ghost
Copy link
Author

ghost commented Dec 25, 2023

I will put this PR up in smaller pieces, as well as investigate blend properties which do not involve scanning the contents.

@ghost ghost closed this Dec 25, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Fix one bug, make ten more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant