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

refactor: Elementize decals. #27104

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

warriorstar-orion
Copy link
Contributor

What Does This PR Do

This PR converts the decal component to an element, handling their appearance and behavior much like /tg/'s (which was initially implemented in tgstation/tgstation#53922 and has been updated regularly since then).

Some signals have been added, namely:

  • COMSIG_ATOM_AFTER_SUCCESSFUL_INITIALIZE, which fires in SSatom after an atom has been initialized.
  • COMSIG_ATOM_DECALS_ROTATING, when SSdcs is given an instruction to remove and re-apply decals on a rotated source.
  • COMSIG_TURF_ON_SHUTTLE_MOVE, which is called when a shuttle turf has successfully transited to its destination.

These are all important for ensuring that decals can respond to changes to their source.

Several defines related to the "strength" of a cleaning action of a turf have been removed, as well as setting the cleanable strength requirement on the decal. This is because they aren't used and don't work; the COMSIG_COMPONENT_CLEAN_ACT signal isn't sent with a strength anywhere. In its place is the cleanable boolean on the decal element, which may come in useful at some point, but which is not used at the moment.

Why It's Good For The Game

  • Number of decal components on Box + Centcomm before change: 1,100
  • Number of decal elements on Box + Centcomm after change: 71

The memory improvements are significant. I would not necessarily expect a performance improvement though it's possible round init time might be a touch faster.

This would be a critical improvement when it comes to spriters moving from baked tile colors to decal overlays, as not doing this would balloon the component count to ~4,000 on Box.

Some notes on alleged "decal performance"

For years my understanding was that there was some unspecified performance issues with using decals instead of baking different tile colors directly into floor icon states. One of my earlier PRs, from late 2021, was tearing out all the decals from old mining station so this has been a point of contention for years: #17260

I've scoured the discord for any mention of what the issues actually were. All I can find is vaguely gesturing at maptick congestion, with no hard numbers in any direction, and suggestions that there may be fewer performance problems now that we have threaded maptick.

In point of fact, /tg/ looked into this extensively in 2020 with tgstation/tgstation#53798. Their conclusion, backed up by Lummox chiming in, was:

There is no way to classify [decals] as a critical performance issue. Sendmaps cost scales mostly with movable count, that is the absolute number one scaling cost of the process, and overlays are not movables.

Spriters are planning to migrate to a decal system similar to TG's, where there's separate decals for every arrangement of filled in squares per color. this would make their lives easier and probably allow for greater player freedom in terms of decorating things. I pushed back initially because of all of the above but frankly I don't like basing decisions like this with no real data or explanation.

If anyone can give me a specific technical explanation of the decal perfomance issues beyond "it was slower", or any hard numbers showing the slowdown or speedup, I'd be very happy. Otherwise I can't think of any reason why we shouldn't go this route.

Images of changes

Testing

  • Spawned in, ensured that decal overlays didn't overlap mobs
  • Crowbarred floor tile, ensured decal overlay was removed
  • Used floor painter, ensured that decal remained after floor was changed
  • Tested mining shuttle on Delta (which rotates), ensured that decals were rotated appropriately
  • Used soap on decal tile, ensured they were not cleaned up
  • Used soap on dirty tile, ensured dirt was cleaned up

Declaration

  • I confirm that I either do not require pre-approval for this PR, or I have obtained such approval and have included a screenshot to demonstrate this below.

Changelog

NPFC

@Alecksohs
Copy link
Contributor

I WILL NOW FILL EVERY HALLWAY WITH DECALS

DECALS FOR EVERYBODY!

@ParadiseSS13-Bot ParadiseSS13-Bot added the -Status: Awaiting type assignment This PR is waiting for its type to be assigned internally label Oct 14, 2024
@Adrer Adrer mentioned this pull request Oct 14, 2024
1 task

/// Remove old decals and apply new decals after rotation as necessary
/datum/controller/subsystem/processing/dcs/proc/rotate_decals(datum/source, old_dir, new_dir)
SIGNAL_HANDLER
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you comment the associated signals?

"desc" = description
)

/datum/element/decal/Attach(atom/target, _icon, _icon_state, _dir, _layer=TURF_LAYER, _alpha=255, _color, _cleanable=FALSE, _description, mutable_appearance/_pic)
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, might be nice to move attach directly under the typedef since it functions more or less as the initializer

Comment on lines +92 to +94
RegisterSignal(target, COMSIG_PARENT_EXAMINE, PROC_REF(examine),TRUE)

RegisterSignal(target, COMSIG_TURF_ON_SHUTTLE_MOVE, PROC_REF(shuttle_move_react),TRUE)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
RegisterSignal(target, COMSIG_PARENT_EXAMINE, PROC_REF(examine),TRUE)
RegisterSignal(target, COMSIG_TURF_ON_SHUTTLE_MOVE, PROC_REF(shuttle_move_react),TRUE)
RegisterSignal(target, COMSIG_PARENT_EXAMINE, PROC_REF(examine), TRUE)
RegisterSignal(target, COMSIG_TURF_ON_SHUTTLE_MOVE, PROC_REF(shuttle_move_react), TRUE)

SIGNAL_HANDLER

source.add_overlay(pic)
// TODO: Fix this disgusting hack
Copy link
Contributor

Choose a reason for hiding this comment

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

noting the todo here

@Henri215 Henri215 added the Refactor This PR will clean up the code but have the same ingame outcome label Oct 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
-Status: Awaiting type assignment This PR is waiting for its type to be assigned internally Refactor This PR will clean up the code but have the same ingame outcome
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants