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 landing lights. #5222

Closed

Conversation

realforest2001
Copy link
Member

@realforest2001 realforest2001 commented Dec 16, 2023

About the pull request

Adds landing lights to ERT landing pads.

Explain why it's good for the game

No one likes getting squished by the ERT shuttles.

Testing Photographs and Procedure

Screenshots & Videos

Put screenshots and videos here with an empty line between the screenshots and the <details> tags.

Changelog

🆑
add: Added landing lights to the ERT landing bays.
imgadd: Added some mapicons for the differend landing light delay settings to make it easier to see what is what on the map.
code: Restructured the procs that toggle landing lights on and off for simplicity.
/:cl:

@realforest2001 realforest2001 marked this pull request as draft December 16, 2023 00:40
@github-actions github-actions bot added Sprites Remove the soul from the game. Mapping did you remember to save in tgm format? Feature Feature coder badge Code Improvement Make the code longer labels Dec 16, 2023
Copy link
Member

@Nanu308 Nanu308 left a comment

Choose a reason for hiding this comment

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

mapping seems fine if le code works :D

@Nanu308 Nanu308 added the Mapping Approved adds 500 new dict keys label Dec 16, 2023
@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label Dec 16, 2023
@cm13-github
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

code/modules/shuttle/shuttle.dm Outdated Show resolved Hide resolved
@cm13-github cm13-github removed the Merge Conflict PR can't be merged because it touched too much code label Dec 17, 2023
@cm13-github
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

code/modules/shuttle/shuttles/dropship.dm Outdated Show resolved Hide resolved
code/modules/shuttle/shuttle.dm Show resolved Hide resolved
code/modules/shuttle/shuttle.dm Outdated Show resolved Hide resolved
@realforest2001 realforest2001 marked this pull request as ready for review December 17, 2023 07:30
@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label Dec 17, 2023
@cm13-github
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@cm13-github
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@cm13-github cm13-github removed the Merge Conflict PR can't be merged because it touched too much code label Dec 18, 2023
@Drulikar Drulikar added the Testmerge Candidate we'll test this while you're asleep and the server has 10 players label Dec 19, 2023
Copy link
Member

@fira fira left a comment

Choose a reason for hiding this comment

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

This seems to break hijack completely, i assume because is_in_endgame is set within crash_site prearrival and now it occurs before the crash

code/modules/shuttle/shuttle.dm Outdated Show resolved Hide resolved
code/modules/shuttle/shuttle.dm Outdated Show resolved Hide resolved
code/modules/shuttle/shuttle.dm Outdated Show resolved Hide resolved
code/modules/shuttle/shuttle.dm Outdated Show resolved Hide resolved
@fira fira marked this pull request as draft December 20, 2023 14:02
Comment on lines +754 to 756
on_prearrival()
return
on_prearrival()
Copy link
Member

Choose a reason for hiding this comment

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

This results in prearrival being called twice for a single landing

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem is the second calling is at the same time as on_arrival and I don't know why it does it. It was doing it prior to my change, it's why the sounds were playing when I was testing but the lights weren't working

Comment on lines 750 to 756
if(prearrivalTime && mode != SHUTTLE_PREARRIVAL)
set_mode(SHUTTLE_PREARRIVAL)
setTimer(prearrivalTime)
log_debug("SHUTTLE DEBUG: shuttle on approach failed check!")
on_prearrival()
return
on_prearrival()
Copy link
Member

Choose a reason for hiding this comment

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

Document modifications from upstream code clearly please, this avoids things getting reverted on update

@@ -704,6 +745,8 @@
if(prearrivalTime && mode != SHUTTLE_PREARRIVAL)
set_mode(SHUTTLE_PREARRIVAL)
setTimer(prearrivalTime)
/// Run on_prearrival after setting the mode rather than waiting for next check, as this doesn't occur until landing is complete.
on_prearrival()
Copy link
Contributor

Choose a reason for hiding this comment

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

All assumptions prior to this PR have expected on_prearrival to occur below on line 708.

As I said before, this needs to be a new proc e.g. on_start_prearrival. Alternatively you do something similar inside of set_mode(SHUTTLE_PREARRIVAL)

Then move all landing light handling that is on_prearrival to the new on_start_prearrival

@cm13-github cm13-github added the Merge Conflict PR can't be merged because it touched too much code label Dec 24, 2023
@cm13-github
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

github-actions bot commented Jan 1, 2024

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 Jan 1, 2024
@cm13-github cm13-github removed the Merge Conflict PR can't be merged because it touched too much code label Jan 2, 2024
@cm13-github
Copy link
Contributor

Conflicts have been resolved. A maintainer will review the pull request shortly.

@github-actions github-actions bot removed the Stale beg a maintainer to review your PR label Jan 3, 2024
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 Jan 10, 2024
@github-actions github-actions bot closed this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Improvement Make the code longer Feature Feature coder badge Mapping Approved adds 500 new dict keys Mapping did you remember to save in tgm format? Sprites Remove the soul from the game. Stale beg a maintainer to review your PR Testmerge Candidate we'll test this while you're asleep and the server has 10 players
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants