-
Notifications
You must be signed in to change notification settings - Fork 28
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
Container tasks #293
base: main
Are you sure you want to change the base?
Container tasks #293
Conversation
HOW TO REVIEW:
|
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.
This is the first change request. I just glanced over the code, and made suggestions.
I also checked out the code, and it seems that if I take all of the items out of the container in one go(ie, I only open the window once), the items end up being on the ground, and when I try to pick them up, they teleport to the centare(0,0) of the map...
I will make another review if I find more bugs.
src/assets/autoload/uimanager.gd
Outdated
@@ -85,6 +87,11 @@ func free_ui(ui_name: String): | |||
push_error("free_ui() called with invalid ui name " + ui_name) | |||
emit_signal("free_ui", ui_name) | |||
|
|||
func pre_ins(ui_name:String): |
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.
what does this function do(maybe a more descriptive name)?
src/assets/main/maps.gd
Outdated
@@ -68,6 +68,8 @@ func switchMap(newMap: String) -> void: | |||
# actual current map to position 0 manually. | |||
move_child(mapClone, 0) | |||
emit_signal("spawn", getSpawnPoints()) | |||
if newMap == "Test": | |||
init_all_tasks() |
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.
This could be moved below line 128 in uimnager, because it is ui based, and that's where I have put open_ui("rolescreen")
.
wait for other peoples input, before you change this one, as I'm not 100% sure myself
src/assets/maps/test/Container.gd
Outdated
|
||
func register(body) -> void:#Register a body | ||
interactor[body.id] = body.get_path() | ||
interactor["bool"] = true |
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.
What does this bool do? more descriptive names can never hurt
src/assets/maps/test/Container.gd
Outdated
if interactor.empty(): | ||
register(bodies) | ||
else: | ||
return |
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.
this can be better structured as
# Can't register a new body, if there is an already registered body
if is_body_registered():
return
for bodies in get_overlapping_bodies():
if not bodies.is_in_group("players"):
continue
register(bodies)
return
func is_body_registered() -> bool:
return interactor.size() > 0
for slot in grid.get_children(): | ||
slot.ui_data.clear() | ||
|
||
puppetsync func erase_child():#erases all child |
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.
erase_children
, plural of child is children
if not get_tree().is_network_server(): | ||
return | ||
rpc("reset") | ||
remotesync func erase_child_server(): |
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.
don't forget to change it to children
here, too... English is sometimes complicated like that(or is it just me? :D)...
@@ -127,3 +130,16 @@ func get_child_node_names() -> Array: | |||
for i in get_children(): | |||
name_list.append(i.name) | |||
return name_list | |||
|
|||
func pre_ins(ui_name: String): |
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.
why not do
func pre_ins(ui_name: String):
instance_ui(ui_name)
close_ui(ui_name)
GameManager.connect('state_changed', self, '_on_state_changed') | ||
|
||
|
||
func _process(_delta):#Called every frame to check interaction status |
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.
is there no way to change this to be event based?
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 have not checked the codebase yet, I trust @Damjan94 that he did a thorough job and I'll look at it after his recommendations have been implemented.
However to expand on the bug @Damjan94 describes in their comment, it happens wit every item you pick up from the storage. Steps to reproduce:
- take an item out of the storage.
- walk away and drop the item on the ground.
- try to pick up the item again
- watch the item move to the center of the map
- the item can not be picked up again.
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.
if you add assert(picked_up_item == null)
before this line, sometimes, when there's a lot of items around the container with the batteries, you end up with a perma item in your hand, that you can't drop.
actually, there don't need to be any items around the container. It sometimes will just put two items if it doesn't close the ui after you take away the first item, and you click to take away another item
map_items.remove_child(self) | ||
if item_from_container: | ||
var path = get_tree().get_root().get_node(item_location) | ||
path.remove_child(self) |
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.
the bug @nicemicro and I found(where items get teleported to (0,0) coordinate) is because item_from_container is never set to false. you could set it to false 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.
I feel like the container.gd is doing too much with respect to synchronizing the data.
I would be more happy if we used an autoload like taskmanager.gd or rather a script attached to main.tscn to handle these kind of synchronization behaviors, as I would think in the future when we have multiple different tasks, having a common back end would be beneficial.
if can_pickup == true: | ||
# Item Handler does not receive input as a child of a Viewport | ||
item_handler._input(event) |
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.
the item_handler should do all the different types of checking whether it is possible to pick up an item or not.
Having a bool for this purpose is not good, because many different events can block pick up, and if two blocking events occur simultaneously, and one is over, that would maybe switch the bool back to true, even though the other blocking event still did not pass.
So instead of having a boolean, just add the additional conditions to item_handler._input()
.
|
||
func _process(_delta):#Called every frame to check interaction status | ||
if not ui_data.empty(): | ||
rpc_id(1, "pass_data_server", ui_data) |
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 also don't understand, what happens if the ui_data becomes empty, when it was not empty before? does it not RPC the server in that case?
Moreover, when this RPC is called, the server will send the RPC to all clients, so it ends up setting it to itself again. Is this intended?
puppetsync func erase_children() -> void:#erases all child | ||
for child in grid.get_children(): | ||
child.queue_free() | ||
|
||
puppetsync func reset() -> void:#Clears all the data | ||
erase_data() | ||
ui_data.clear() |
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.
shouldn't we check here whether the RPC is coming for the server?
Container refactor: adding the interaction resource to the map
…o container-tasks
renaming files
…suspect into container-tasks
added new items for the chemical cabinet
A container task as intended by the design document
Currently it is fully functional/completed From Game Perspective but enough for getting items from the box
This PR provides:-