Movecraft icon indicating copy to clipboard operation
Movecraft copied to clipboard

[Feature / Cleanup] Sign rework (WIP)

Open DerToaster98 opened this issue 1 year ago • 3 comments

Describe in detail what your pull request accomplishes

This PR is a attempt at redoing signs in a more organized way. That encompasses the following things:

  • Signs get registered to a central registry / map
  • A single listener retrieves the fitting registered sign and calls those for processing
  • Signs can be overridden, useful for addons that want to change behaviors of let's say move signs (e.g. Squadrons reloaded)
  • Removes redundant code and should make it cleaner
  • Should make remote sign implementations much cleaner cause that way one can just call the relevant registered sign instead of firing a new event

TODO List:

  • [x] Base class for signs
  • [x] Base class for "craft signs" (everything that requires a craft and that can react to craft detection and sign translation)
  • [x] Sign registration
  • [x] Sign retrieval by "ID"
  • [x] Base cruise sign
  • [x] Base information sign
  • [x] AbstractCruiseSign => will be the base class for signs acting similar to the cruise sign
  • [x] Abstraction layer for signs
  • [x] 1.20+: Support for sided signs
  • [x] Deprecation notice and comment in all relevant places that use the SignWrapper as discussedin #688
  • [x] Change SignTranslateEvent to use the SignWrapper
  • [x] Subcraft base sign
  • [x] Re-Implement existing signs
    • [x] Ascend sign
    • [x] Descend sign
    • [x] Cruise sign
    • [x] Helm sign
    • [x] Name sign
    • [x] Move sign
    • [x] Pilot sign => TODO: Should we replace the PilotValidator in the detection task or not?
    • [x] RelativeMove sign
    • [x] Release sign
    • [x] Remote sign => will receive rework to better handle multiple target signs and to avoid beavering => Not possible without rewriting how the release stuff works
    • [x] Scuttle sign
    • [x] Speed sign
    • [x] SubcraftRotate => will maybe receive a rework so it is more reliable and less prone to beavering
    • [x] Teleport sign
    • [x] Status sign
    • [x] Contacts sign
    • ~~[ ] NEW Subcraft Move~~ Postpone for later, this requires thorough analysis prior

Checklist

  • [ ] Proper internationalization
  • [ ] Tested
  • [ ] Performance tested (could improve performance on remote signs and subcraft, not done yet)

DerToaster98 avatar Aug 01 '24 17:08 DerToaster98

  * [ ]  **NEW** Subcraft Move

This would be best left for a separate PR. I believe there are many assumptions that Subcrafts never move, so we'll have to do a through review of the codebase for that feature.

TylerS1066 avatar Aug 04 '24 12:08 TylerS1066

  * [ ]  **NEW** Subcraft Move

This would be best left for a separate PR. I believe there are many assumptions that Subcrafts never move, so we'll have to do a through review of the codebase for that feature.

Alright, i will remove that from the todo list. Will implement the other points nonetheless

DerToaster98 avatar Aug 04 '24 13:08 DerToaster98

I've not gotten even an understanding of how this PR works yet, but I have a few comments on the first few files I've looked through.

Github says there are still open reviews, i can't see them though. I will now try to fix the merge conflicts.

After that this PR is ready for testing and approval

DerToaster98 avatar Aug 29 '24 10:08 DerToaster98