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

Add MapTowny integration! #556

Closed
wants to merge 3 commits into from
Closed

Conversation

silverwolfg11
Copy link
Collaborator

Description:

This PR aims to integrate MapTowny with SiegeWar!

The MapTowny integration is functionally the same as the Dynmap / Dynmap-Towny integration and copies some code from the integration as well.

Since MapTowny works with various platforms, there were no default icons, so a fire icon had to be added into the jar to render it on the map. This icon is licensed with attribution, so attribution was added to the README.


New Nodes/Commands/ConfigOptions:

N/A


Relevant Issue ticket:

#283


  • I have tested this pull request for defects on a server.
    Tested on a local testing server with an incomplete siege. Would appreciate if people with more experience with SiegeWar and/or players test this as well.

By making this pull request, I represent that I have the right to waive copyright and related rights to my contribution, and agree that all copyright and related rights in my contributions are waived, and I acknowledge that the TownyAdvanced organization has the copyright to use and modify my contribution under the SiegeWar License for perpetuity.

- Functionally the same as the Dynmap / Dynmap-Towny integration.
- Uses code from the Dynmap Integration to display siege icon markers.
- Add a specific fire icon to use for MapTowny and attribution for the icon in the README.
@Goosius1 Goosius1 added the enhancement New feature or request label Apr 28, 2022
@Goosius1 Goosius1 added this to the 0.7.15 milestone Apr 28, 2022
@Goosius1 Goosius1 self-requested a review April 28, 2022 17:51
@Goosius1
Copy link
Collaborator

Thanks so much for adding this! Huzzah!!!.
Just 1 cr comment

@LlmDl
Copy link
Member

LlmDl commented Apr 28, 2022

I mentioned this to @silverwolfg11 in #towny-dev as well but it ought to go here too: the formatting on the existing files will have to be undone too. He says this is a bit of a proof of concept first, while he considers what other API needs to be added to MapTowny.

@Goosius1 Goosius1 modified the milestones: 0.7.15, 0.8.0 Apr 29, 2022
@silverwolfg11
Copy link
Collaborator Author

Fixed the line endings and made the map change to be triggered the NewShortTimeEvent instead of a fixed interval.


@EventHandler
public void onNewShortTime(NewShortTimeEvent event) {
this.renderSieges.run();
Copy link
Collaborator

@Goosius1 Goosius1 May 1, 2022

Choose a reason for hiding this comment

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

🚧

  • Have I missed something, or is threading unnecessary here?
  • Couldn't we just do something un-threaded like: SiegeWar.getMapTownyIntegration().displaySieges() ?

Copy link
Collaborator

@Goosius1 Goosius1 left a comment

Choose a reason for hiding this comment

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

1 Comment

@Goosius1 Goosius1 removed this from the 0.8.0 milestone May 11, 2022
@Goosius1
Copy link
Collaborator

Hi Silverwolf, what is the status of this PR?

@Goosius1
Copy link
Collaborator

Hi @silverwolfg11 , what is the status of this PR?

@Goosius1
Copy link
Collaborator

Goosius1 commented Jul 17, 2023

  • This PR has been here more than a year without activity.
  • Gonna do a little cleanup now and close it
  • If the work picks up again on this, this PR can be re-opened or, maybe more appropriately, a different PR, as the many SW changes in the last year would need to be accounted for.

@Goosius1 Goosius1 closed this Jul 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants