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

Docs: Dev FAQ - About indirect conditions #3698

Closed
wants to merge 19 commits into from
Closed

Conversation

NewSoupVi
Copy link
Member

@NewSoupVi NewSoupVi commented Jul 27, 2024

I wrote up a big effortpost about indirect conditions for nex on the DS3 3.0 PR.

The version I'm PRing to the world API document is very brief and unnuanced, because I'd rather people use too many indirect conditions than too few. But that might leave some devs wanting to know more.

I think that comment on nex's DS3 PR is probably the best detailed explanation for indirect conditions that exists currently.

So I think it's good if it exists somewhere. And the FAQ doc seems like the best place right now, because I don't want to write an entirely new doc at the moment.

I wrote up a big effortpost about indirect conditions for nex on the [DS3 3.0 PR](#3128 (comment)).

The version I'm [PRing to the world API document](#3552) is very brief and unnuanced, because I'd rather people use too many indirect conditions than too few.
But that might leave some devs wanting to know more.

I think that comment on nex's DS3 PR is probably the best detailed explanation for indirect conditions that exists currently.

So I think it's good if it exists somewhere. And the FAQ doc seems like the best place right now, because I don't want to write an entirely new doc at the moment.
@github-actions github-actions bot added is: documentation Improvements or additions to documentation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet. labels Jul 27, 2024
@NewSoupVi NewSoupVi requested a review from Exempt-Medic July 27, 2024 06:30
@NewSoupVi
Copy link
Member Author

NewSoupVi commented Jul 27, 2024

Requesting a review from Exempt-Medic bc I have a lot of faith in their understanding of the issue & ability to write things

@black-sliver
Copy link
Member

black-sliver commented Jul 27, 2024

Im wondering why you chose not to paste the text into the doc?

@Exempt-Medic
Copy link
Member

I agree with Black Sliver. I think it would make sense to port in the text, at least just to space/format it well

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Jul 27, 2024

That's fair. I didn't have any more motivation to do any more after writing that comment, so I just opened the simplest PR I could think of so it doesn't fall by the wayside. But yeah, you guys are right. I'll get to that

@NewSoupVi NewSoupVi marked this pull request as draft July 31, 2024 08:54
@NewSoupVi NewSoupVi marked this pull request as ready for review July 31, 2024 17:35
@NewSoupVi
Copy link
Member Author

Copied in the text. Idk, it wasn't really written for a doc. If anyone wants to rewrite this to make more sense as a doc text, feel free

docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
NewSoupVi and others added 5 commits July 31, 2024 22:33
Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

LGTM and reads well

Copy link
Collaborator

@ScipioWright ScipioWright left a comment

Choose a reason for hiding this comment

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

Still discussing this internally, but a couple changes while I have a moment

docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
NewSoupVi and others added 2 commits August 13, 2024 18:18
Copy link
Contributor

@qwint qwint left a comment

Choose a reason for hiding this comment

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

All my inline suggestions fall under the umbrella of 'the wording still reads too much like a conversation between Vi and someone else' rather than an entry in a doc, it doesn't need crazy rewording just some points which can be more authoritative about what is true at this point in time rather than explaining how we got here

That being said, scipio and I discussed (and i almost forgot about it) this having a new section with a header along the lines of

## Extra Information to Help Development

that this section could fall under, because all of the other sections are about best practices in code while this section is different being more of an informational explanation

docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
@NewSoupVi
Copy link
Member Author

Wrote new text, so this should be rereviewed now

Copy link
Member

@Exempt-Medic Exempt-Medic left a comment

Choose a reason for hiding this comment

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

Some comments for now

docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
The reason entrance access rules using `location.can_reach` and `entrance.can_reach` are also affected is simple: They call `region.can_reach` on their respective parent/source region.

We recognize it can feel like a trap since it will not alert you when you are missing an indirect condition, and that some games have very complex access rules.
As of [PR #3682 (Core: Region handling customization)](https://github.com/ArchipelagoMW/Archipelago/pull/3682) being merged, it is also possible for a world to opt out of indirect conditions entirely, although it does come at a flat performance cost.
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird to me to call this opting out instead of opting into having your regions always rechecked

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is probably more intuitive? Idk. Willing to consider a suggestion but I think rn I'd keep it

docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
To account for this case, AP would have to recheck all entrances every time a new region is reached, until no new regions are reached.

However, there is a way to **manually** define that a *specific* entrance needs to be rechecked during region sweep if a *specific* region is reached during it. This is what an indirect condition is.
This keeps almost all of the performance upsides. Even a game making heavy use of indirect conditions (See: The Witness) is still way way faster than if it just blanket "rechecked all entrances until nothing new is found".
Copy link
Member

Choose a reason for hiding this comment

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

It seems weird ro say "the performance upsides" when they aren't mentioned anywhere. I think some of the structure/paragraphing here jumps around from topic quickly in ways thar carry assumptions about the dev's knowing why the subject is changing. I don't think how it is now is particularly bad but I'd have to think a good bit to come up with a structure I think would be better

Copy link
Member Author

Choose a reason for hiding this comment

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

There is "For performance reasons, ..." some paragraphs before this
Might not be direct enough, idk

docs/apworld_dev_faq.md Outdated Show resolved Hide resolved
docs/apworld_dev_faq.md Outdated Show resolved Hide resolved

Region sweep (the algorithm that determines which regions are reachable) is a Breadth-First Search of the region graph from the origin region, checking entrances one by one and adding newly reached nodes (regions) and their entrances to the queue until there is nothing more to check.

For performance reasons, AP only checks every entrance once. However, if entrance access conditions depend on regions, then it is possible for this to happen:
Copy link
Contributor

Choose a reason for hiding this comment

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

rewriting the suggestion because it technically had a conflict lol

Suggested change
For performance reasons, AP only checks every entrance once. However, if entrance access conditions depend on regions, then it is possible for this to happen:
For performance reasons, AP only checks every entrance once. However, if an entrance's access condition depends on regions, then it is possible for this to happen:
Suggested change
For performance reasons, AP only checks every entrance once. However, if entrance access conditions depend on regions, then it is possible for this to happen:
For performance reasons, AP only checks every entrance once. However, if any entrance access condition depends on regions, then it is possible for this to happen:
Suggested change
For performance reasons, AP only checks every entrance once. However, if entrance access conditions depend on regions, then it is possible for this to happen:
For performance reasons, AP only checks every entrance once. However, if entrance Access Conditions depend on regions, then it is possible for this to happen:
Suggested change
For performance reasons, AP only checks every entrance once. However, if entrance access conditions depend on regions, then it is possible for this to happen:
For performance reasons, AP only checks every entrance once. However, if entrance access_conditions depend on regions, then it is possible for this to happen:

giving a couple more options too, because I don't think there's one way to fix this, but as-is it reads strange without the assumption that "access condition" is one idea and not multiple words you have to parse their role in the sentence
easiest is probably to rewrite the whole sentence, but i think any of these help 😅

@qwint
Copy link
Contributor

qwint commented Nov 25, 2024

I know the heading mentions coming from the world_api doc which does mention the function,
but I (literally seconds ago) pointed a dev to this doc to describe indirect connections and how to fix them explicitly and they came back with "I'm not seeing where I would define each entrance to have this property" because the current text doesn't mention the functions at all,,

this doc should probably mention register_indirect_condition at the very least to allow it to stand alone as something that can be linked to

@NewSoupVi
Copy link
Member Author

NewSoupVi commented Nov 26, 2024

For the sake of this PR being able to have a future:

Y'all have my full permission to rewrite and add to this as you please, I relinquish all claims to it.

The reason I made this PR is because I had already written the whole thing and it seemed like it might be useful to others. For personal time / motivation reasons, I am not interested in iteratively improving it via review process.

Ideally, someone (e.g. qwint) can just copy these changes and reopen the same PR. Then I can also merge it when it's done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
is: documentation Improvements or additions to documentation. waiting-on: peer-review Issue/PR has not been reviewed by enough people yet.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants