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

Links Awakening: Implement New Game #1334

Merged
merged 16 commits into from
Mar 20, 2023

Conversation

zig-for
Copy link
Collaborator

@zig-for zig-for commented Dec 18, 2022

What is this fixing or adding?

Adds Link's Awakening: DX. Fully imports and forks LADXR, with permission - https://github.com/daid/LADXR

How was this tested?

Multiple beta tests. No major issues have been revealed over the past few ones.

Known bugs:

  • Check count at the end screen could be off by 5, due to local only items using a special snowflake collection method - will fix in later revision
  • Client hangs when waiting on player to actually collect an item
  • One person was unable to connect to Retroarch, still a mystery why, need more data

@zig-for zig-for changed the title Links awakening Links Awakening: Draft Dec 18, 2022
@zig-for zig-for changed the title Links Awakening: Draft Links Awakening: Implement New Game Feb 4, 2023
@zig-for zig-for marked this pull request as ready for review February 5, 2023 23:47
worlds/ladx/LADXR/.github/workflows/multiworld_build.yml Outdated Show resolved Hide resolved
worlds/ladx/LADXR/.gitignore Outdated Show resolved Hide resolved
worlds/ladx/LADXR/LICENSE Outdated Show resolved Hide resolved
@zig-for zig-for requested a review from Jarno458 February 7, 2023 00:52
Copy link
Collaborator

@Jarno458 Jarno458 left a comment

Choose a reason for hiding this comment

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

Tnx for the cleanup

Copy link
Contributor

@SoldierofOrder SoldierofOrder left a comment

Choose a reason for hiding this comment

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

Only examined the two user-facing docs files (setup guide and info page). Very thorough and clear writing!

worlds/ladx/docs/en_Links Awakening DX.md Outdated Show resolved Hide resolved
worlds/ladx/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/ladx/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/ladx/docs/setup_en.md Outdated Show resolved Hide resolved
worlds/ladx/Items.py Outdated Show resolved Hide resolved
worlds/ladx/Options.py Outdated Show resolved Hide resolved
host.yaml Outdated
@@ -82,7 +82,7 @@ generator:
race: 0
# List of options that can be plando'd. Can be combined, for example "bosses, items"
# Available options: bosses, items, texts, connections
plando_options: "bosses"
plando_options: "items"
Copy link
Member

Choose a reason for hiding this comment

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

why is this change here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to change it during a beta test, accidentally committed. Will do another pass tonight for small things like this I've missed.

@black-sliver black-sliver added the is: new game Pull requests for implementing new games into Archipelago. label Feb 27, 2023
Network Command Port at 55355.

![Screenshot of Network Commands setting](/static/generated/docs/A%20Link%20to%20the%20Past/retroarch-network-commands-en.png)
4. Go to Main Menu --> Online Updater --> Core Downloader. Scroll down and select "Nintendo - SNES / SFC (bsnes-mercury
Copy link
Member

Choose a reason for hiding this comment

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

This seems like the wrong core

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed


This randomizer is based on (forked from) the wonderful work daid did on LADXR - https://github.com/daid/LADXR

The autotracker code for communication with magpie tracker is directly copied from kbranch's repo - https://github.com/kbranch/Magpie/tree/master/autotracking
Copy link
Collaborator

Choose a reason for hiding this comment

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

So you copied from a repository that does not come with a license, but which says it has been ported from yet another repository that also does not grant us any license whatsoever? And you are now passing that off as MIT.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll get kbranch to supply a license. He gave permission, I promise. :) Good catch.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Resolved.

Copy link

Choose a reason for hiding this comment

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

Zig definitely asked for permission before touching anything, for what it's worth. I thought I had added an MIT LICENSE file, but apparently I forgot it - I just added it.

Magpie also doesn't use any actual code from the old Emotracker pack, so anything he uses is safe. If there are still concerns, I'm sure MuffinJets would happily make it official in his repo as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks for releasing it as MIT.

Choose a reason for hiding this comment

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

I can't speak for how EmoTracker as a core applies here, but if Magpie has any leftover code from my work, I give full permission and will apply MIT if needed

worlds/ladx/docs/setup_en.md Outdated Show resolved Hide resolved

class LinksAwakeningContext(CommonContext):
tags = {"AP"}
game = "Links Awakening DX" # empty matches any game since 0.3.2
Copy link
Member

Choose a reason for hiding this comment

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

these comments are copied from text clients and all 3 don't apply to this client

ladxr_name = "tradequest"


# Setting('forwardfactor', 'Main', 'F', 'Forward Factor', default=0.0,
Copy link
Member

Choose a reason for hiding this comment

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

looks like leftover stuff that could be removed? I found a bunch of single line old code comments as well, but this one is a bit big

# TODO: translate to lttp parlance
class EntranceShuffle(Choice, LADXROption):
"""
[WARNING] Experimental, may break generation
Copy link
Member

Choose a reason for hiding this comment

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

What does "break generation" mean in AP context?

Copy link
Collaborator

Choose a reason for hiding this comment

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

From conversations, I believe that it will sometimes generate entrance layouts that isolate groups of checks, which is not accounted for, and it will fail accessibility checks.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Confirmed with zig this is the case.

Copy link
Member

Choose a reason for hiding this comment

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

I believe SM can do that too, in which case it forces accessibility minimal

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll verify this tonight, but when I checked with zig about this, they mentioned that EntranceShuffle is disabled for this first version anyway.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could search for unreachable locations under an "all state" and delete them, maybe? Or if they have events / prefilled advancement items, set those items to filler classification. Then it should be able to succeed under either items or minimal accessibility?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe a proper solution to prevent isolated locations from occurring in the first place is in the works for a future update.

Comment on lines 78 to 80
def create_item(self, item: str) -> LinksAwakeningItem:
assert(False)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def create_item(self, item: str) -> LinksAwakeningItem:
assert(False)

This is defined again further down, so this can go away.


def modify_multidata(self, multidata: dict):
multi_key = binascii.hexlify(self.generate_multi_key()).decode()
multidata["connect_names"][multi_key] = multidata["connect_names"][self.multiworld.player_name[self.player]]
Copy link
Member

Choose a reason for hiding this comment

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

I see you encode a connect name, but also allow the client user entry of the player name?
I think that makes this the first game to do that, normally if a connect hash is encoded, that's the only way to connect with that game's client.

[Hell] Obscure knowledge and hard techniques may be required. Examples include featherless jumping with boots and/or hookshot, sequential pit buffers and unclipped superjumps. Things in here can be extremely hard to do or very time consuming."""
display_name = "Logic"
ladxr_name = "logic"
# option_casual = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Option is described in description but is disabled

Copy link
Collaborator

Choose a reason for hiding this comment

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

Resolved

@Berserker66 Berserker66 merged commit 81a2393 into ArchipelagoMW:main Mar 20, 2023
kindasneaki pushed a commit to kindasneaki/Archipelago that referenced this pull request Jun 28, 2023
Adds Link's Awakening: DX. Fully imports and forks LADXR, with permission - https://github.com/daid/LADXR
FlySniper pushed a commit to FlySniper/Archipelago that referenced this pull request Nov 14, 2023
Adds Link's Awakening: DX. Fully imports and forks LADXR, with permission - https://github.com/daid/LADXR
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: new game Pull requests for implementing new games into Archipelago.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants