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

Overlay fixes #6476

Merged
merged 2 commits into from
May 15, 2021
Merged

Overlay fixes #6476

merged 2 commits into from
May 15, 2021

Conversation

Semuca
Copy link
Contributor

@Semuca Semuca commented May 9, 2021

Purpose

This PR fixes bugs that I noticed while looking at plasma fires, like player overlays, tile overlays, and Unity throwing modified collection errors in TileList.cs.

Explanation:

Screenshot 2021-05-09 194229

Overlays are now stored under the sprite gameobject in players, so they rotate whenever the player body rotates. This fixes #6429.

Tile overlays like windows smashing were added too many times because the system was not checking if an overlay was already added correctly. I overwrote the .Equals() function so they can compare in a more functional way.

Tile overlays now have a const which determines the maximum overlays allowed per tile, and some weird code has been cleaned up. Previously, the z position of overlays was increased in this order: 1, 2, 4, 7, 11. Now, they'll increase in a normal ascending order (1, 2, 3, etc).

Changelog:

CL: Fixed fire and electrocution overlay not matching the player's rotation
CL: Fixed tiles having too many overlays

@ThatDan123
Copy link
Contributor

"some weird code" who would do such a thing, thanks for fixing it.

@corp-0 corp-0 added the Status: Awaiting Review for PRs that are awaiting reviews/rereviews label May 9, 2021
@corp-0
Copy link
Member

corp-0 commented May 9, 2021

so they rotate whenever the player body rotates

shouldn't flames always be upright?

UnityProject/Assets/Scripts/Player/PlayerSprites.cs Outdated Show resolved Hide resolved

if (Layers.TryGetValue(layerType, out var layer))
{
//Only check 20 as its unlikely to ever have more than 20 overlays
//Go through overlays under the overlay limit. The first overlay checked will be at z = 1.
Copy link
Member

Choose a reason for hiding this comment

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

this loop seems to be used a lot and only slightly different each time. Can you extract it into another method to avoid duping code?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not really, because each loop does something different depending on the function it's in

Copy link
Member

Choose a reason for hiding this comment

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

yes, just slightly different and as you could experience yourself now when you had to make some changes, you had to do the same changes in all loops. Try extracting the part that is identical in all instances, return what the different methods need and operate with that returned value, that way if we need to change the loop we only change it in one place.

Copy link
Contributor

@NoooneyDude NoooneyDude May 11, 2021

Choose a reason for hiding this comment

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

I would suggest a Func delegate (take to PlayerList getInternal() and its callers for an example) however I suspect it creates garbage.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually should be fine if you only create the delegate objects once.

@Semuca
Copy link
Contributor Author

Semuca commented May 10, 2021

so they rotate whenever the player body rotates

shouldn't flames always be upright?

But otherwise when the player drops the outline of the player on fire will just be left standing. If you mean wanting to have the fire rise up no matter what rotation the player is at, that's quite a big task which to me isn't all that noticeable. For now I think just rotating the overlay with the player is fine.

@NoooneyDude
Copy link
Contributor

NoooneyDude commented May 11, 2021

so they rotate whenever the player body rotates

shouldn't flames always be upright?

But otherwise when the player drops the outline of the player on fire will just be left standing. If you mean wanting to have the fire rise up no matter what rotation the player is at, that's quite a big task which to me isn't all that noticeable. For now I think just rotating the overlay with the player is fine.

Yeah some sort of dynamic flame sprite generation based on the current object sprite would be nice. Could be split into individual, small flame sprites determined by fire stacks and object sprite existence in that area. Could get pretty complicated.

@NoooneyDude NoooneyDude added Status: Awaiting Changes Something about this PR should be / was requested to be changed before merging and removed Status: Awaiting Review for PRs that are awaiting reviews/rereviews labels May 11, 2021
@corp-0
Copy link
Member

corp-0 commented May 12, 2021

@Semuca do you think you can make the requested changes? Let us know if you find any trouble when trying.

@Semuca
Copy link
Contributor Author

Semuca commented May 13, 2021

Sorry about all that, I was up to my neck in schoolwork. I need to learn about delegate functions, but that shouldn't be too much. I'll see if I can do that change on Friday.

@NoooneyDude
Copy link
Contributor

Sorry about all that, I was up to my neck in schoolwork. I need to learn about delegate functions, but that shouldn't be too much. I'll see if I can do that change on Friday.

I've decided to merge this now - probably shouldn't keep player prefab in limbo. A followup PR with the refactor would be greatly appreciated, still.

@NoooneyDude NoooneyDude merged commit eaa46f2 into unitystation:develop May 15, 2021
@Semuca Semuca deleted the overlayfixes branch May 16, 2021 12:01
@PerfectTangent PerfectTangent removed the Status: Awaiting Changes Something about this PR should be / was requested to be changed before merging label Jan 9, 2022
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.

Overlays don't match sprite rotation
6 participants