-
Notifications
You must be signed in to change notification settings - Fork 19
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
Potion class, and how it is encoded #86
base: v2.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great PR! I made a lot of suggestions, feel free to reply to any of them if you have follow up questions.
One additional thing – please remove the LICENSE and .DS_Store files
README.md
Outdated
1) Know the attributes of your object (duh) | ||
|
||
2) Go to **builtin.py** -> **Obj** and find **Obj** class. This is where you initialize the object. **NOTE:** You don't assume the player is going to use an object. This is merely initialization. Tip: Initialize the logic of your object here. | ||
|
||
3) The hard part is done - go nuts and build your own class definition. | ||
|
||
4) Do not forget to go to **crpg.py** -> **generate()** iff you are: | ||
- creating a new object type. | ||
- go to the first for loop and look for the first if statement. | ||
- remember to include your new object type in n_types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should steer away from instructions on modifying the code itself. Instead, we can comment above and within the functions we write so that devs can understand exactly what each function does!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent improvements! It's almost ready to go, once these things are changes I'm ready to merge :)
builtin.py
Outdated
if "-health-collect" in title: | ||
self.amount_health = int(title[19:21]) | ||
self.potionType = "Health potion" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we have classes that inherit the potion class for Health, Speed, etc, instead of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it. Check out the recent push. I made it as generic as possible, but notice that it is impossible to encode "this is a health potion worth 25 hp" without actually calling it out and subsequently using conditionals to decipher it. However, the implementation of Potion is certainly cleaner now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add is_health_potion as an argument? Another approach would be to have an action method with different implementations for each class
crpg.py
Outdated
@@ -99,7 +100,7 @@ def generate(filename): | |||
for node in nodes.split("\n"): | |||
n, n_type, *args = re.split(r"\s*\|\s*", node) | |||
|
|||
if n_type == "fight" or n_type == "run": | |||
if n_type == "fight" or n_type == "run" or n_type[:2] == "po": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should be able to specify n_type == "potion" here. n_type stands for node type, and is the second item in the .dl file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
README.md
Outdated
1) Know the attributes of your potion | ||
|
||
2) Go to **builtin.py** -> **Potion** and find **Potion** class. This is where you initialize the potion. **NOTE:** You don't assume the player is going to use an object. This is merely initialization. Tip: Initialize the logic of your object here. | ||
|
||
3) Do not forget to go to **crpg.py** -> **generate()** iff you are: | ||
- creating a new potion type. | ||
- go to the first for loop and look for the first if statement. | ||
- remember to include your new object type in n_types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this documentation is a bit out of place here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Deleted.
…ay all items in the bag as a count of each type of object
…sociated with the potion
Proposed changes
All changes and expected Object implementation procedure are outlined in the README. A working example of how to use the currency and potion objects in the .dl file is demonstrated in the .dl file. Note that the logic in the current demonstration of the proposed changes only show the usability of the classes; in reality you'd want to sever the connection to a money object after its been visited but I didn't do that, although a simple fix would be to just change the arrow type.
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that applyChecklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. This is simply a reminder of what we are going to look for before merging your code.Further comments
Its not the most elegant way to encode potions, but due to the nature of having many types of potions and the way we currently have the .dl file set up, our options are limited. Also, I forgot to revert the changes I made to .dl, do not forget to add it back after merging.