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

New event style #218

Open
wants to merge 22 commits into
base: main
Choose a base branch
from
Open

New event style #218

wants to merge 22 commits into from

Conversation

HakiRose
Copy link
Contributor

The Traceables!

@HakiRose HakiRose requested a review from MarcCote February 24, 2020 20:06
…- Push_button and Cloth are assumed simply as key and object, respectively.
author HakiRose <[email protected]> 1567777575 -0400
committer HakiRose <[email protected]> 1582743626 -0500

Medium-level Spaceship game -- cloth(finished), Push button (in progress)
…l) is implemented, revisions on FW for stochastic commands, some minor changes on a few other files.
… and refactoring, game files(json, ni, ulx), updated logic files, ...
… adapt the new Predicate, Proposition, & Signature styles. This new framework can track a proposition through the time.
…w quest/Event design, new world2inform7 structure, and more.
…opments on test files, some updates on the frame work, semi-final updates on Content Check Game, etc.
Copy link
Contributor

@MarcCote MarcCote left a comment

Choose a reason for hiding this comment

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

@HakiRose I've made a first pass. Next, I'll dig into game.py and logic/__init__.py. But before I do so, can you confirm the code still needs "verb" and "definition" in Proposition?

state.remove_fact(prop)

def has_traceable(self):
for prop in self.get_facts():
Copy link
Contributor

Choose a reason for hiding this comment

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

This class doesn't seem to have a method called get_facts.

@@ -63,22 +69,86 @@ def _get_name_mapping(action):
return commands


class Event:
class PropositionControl:
Copy link
Contributor

Choose a reason for hiding this comment

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

This class doesn't seem to have corresponding unit tests.

@@ -117,41 +117,60 @@ def _variable_(self): # noqa
[]
)

def _predVT_(self, name):
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is automatically generated by TatSu depending on the content of textworld/logic/logic.ebnf upon making a new TextWorld release. This means you shouldn't/can't modify it directly. Instead, you must modify logic.ebnf accordingly.

@@ -37,11 +37,15 @@ class VariableNode(ModelBase):
class SignatureNode(ModelBase):
name = None
types = None
verb = None
Copy link
Contributor

Choose a reason for hiding this comment

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

This file is automatically generated by TatSu depending on the content of textworld/logic/logic.ebnf upon making a new TextWorld release. This means you shouldn't/can't modify it directly. Instead, you must modify logic.ebnf accordingly.

@@ -266,7 +266,7 @@ def _process_rooms(self) -> None:

# Handle door link facts.
for fact in self.facts:
if fact.name != "link":
if fact.name != "is__link":
Copy link
Contributor

Choose a reason for hiding this comment

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

For backward compatibility (very important), we should support not having an is__ by default.

Comment on lines +220 to +222
for ph, var in mapping.items():
a = ph.name
b = self.entity_infos[var.name].name
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem to be used.

@@ -1928,3 +2051,20 @@ def __str__(self):
lines.append("})")

return "\n".join(lines)

def get_facts(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference with the property .facts?

@@ -24,3 +24,4 @@ tmp/*
*.ipynb_checkpoints
/dist
/wheelhouse
*.orig
Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea. The branch associated with this PR already contains a bunch of .orig files, though.

@@ -0,0 +1,341 @@
from collections import defaultdict
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's remove the spaceship challenge from this PR to keep the changes manageable (for me) to review ;)

@@ -149,19 +149,19 @@ def make_quest(world: Union[World, State], options: Optional[GameOptions] = None
for i in range(1, len(chain.nodes)):
actions.append(chain.actions[i - 1])
if chain.nodes[i].breadth != chain.nodes[i - 1].breadth:
event = Event(actions)
event = EventCondition(actions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Since actions is not the first parameters of the constructor anymore, you need to write actions=actions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants