Jonathan
/
Babushka
2
0
Fork 0
Code Pull Requests 3 Releases Activity
Labels Milestones

feature/showcase_bugfixing_kathi_partII #16

Merged
Jonathan merged 33 commits from feature/showcase_bugfixing_kathi_partII into develop 3 months ago
Conversation 0 Commits 33 Files Changed 194
kziolkowski commented 5 months ago
Owner

Merged Quest branch and integrated further bugfixes. Tomato farming works again.

  • changed initial quest post it text and added word wrapping
  • fixed farming animations (farming, watering, pickup) and changed watering can fillstate display to a slider
  • made fields independent of plant, making famring system more robust
  • made all interaction areas also work with mouse click as input
  • added interaction wrapper that reacts to inventory selection status
  • added hotkey-label to inventory
Merged Quest branch and integrated further bugfixes. Tomato farming works again. - changed initial quest post it text and added word wrapping - fixed farming animations (farming, watering, pickup) and changed watering can fillstate display to a slider - made fields independent of plant, making famring system more robust - made all interaction areas also work with mouse click as input - added interaction wrapper that reacts to inventory selection status - added hotkey-label to inventory
kziolkowski added 14 commits 5 months ago
kziolkowski requested review from Jonathan 5 months ago
Jonathan reviewed 5 months ago
prefabs/UI/Inventory/Inventory.tscn
grow_vertical = 2
theme_override_colors/font_color = Color(0, 0, 0, 1)
text = "Switch to Unity"
text = "[Hier könnte Ihre Werbung stehen]"
Jonathan commented 5 months ago
Owner

😂

😂
kziolkowski marked this conversation as resolved
Jonathan requested changes 5 months ago
Jonathan left a comment
Owner

Aktuell sind noch ein paar Sachen kaputt. Daher würde ich es noch nicht mergen. Ggf. mergen wir das erst nach der Gamescom und machen aus dem aktuellen Branch nach Bedarf Builds.

Aktuell sind noch ein paar Sachen kaputt. Daher würde ich es noch nicht mergen. Ggf. mergen wir das erst nach der Gamescom und machen aus dem aktuellen Branch nach Bedarf Builds.
scenes/Babushka_scene_farm_outside_2d.tscn Outdated
script = ExtResource("66_2065p")
questResource = ExtResource("67_tm0yg")
toStatus = 1
makeActive = true
Jonathan commented 5 months ago
Owner

Durch die Änderung wird die erste Quest nicht richtig getriggert und das Post-it zeigt nur den Standardtext.

Durch die Änderung wird die erste Quest nicht richtig getriggert und das Post-it zeigt nur den Standardtext.
kziolkowski marked this conversation as resolved
scenes/Babushka_scene_farm_outside_2d.tscn Outdated
[node name="Yeli" parent="YSorted" instance=ExtResource("24_wtdui")]
position = Vector2(6403, 3362)
_timelinesToPlay = PackedStringArray("yeli_quest_select")
_retriggerSameTimeline = true
Jonathan commented 5 months ago
Owner

Dadurch ist Yelis Dialog kaputt.

Dadurch ist Yelis Dialog kaputt.
Jonathan commented 5 months ago
Owner

Yelis Dialog ist gerade komplett kaputt. Bei mir zumindest wird nie "Please could you get the runner ducks back into their coop?" erreicht und auch nicht die zweite Quest aktiviert. Kannst du den gesamten Questdurchlauf bei dir mal gegenchecken?

Yelis Dialog ist gerade komplett kaputt. Bei mir zumindest wird nie "Please could you get the runner ducks back into their coop?" erreicht und auch nicht die zweite Quest aktiviert. Kannst du den gesamten Questdurchlauf bei dir mal gegenchecken?
kziolkowski marked this conversation as resolved
scripts/CSharp/Common/Farming/FarmingControls2D.cs Outdated
}
#endregion
Jonathan commented 5 months ago
Owner

Nen change ohne changes. Währe etwas sauberer, wenn hier garkein change währe

Nen change ohne changes. Währe etwas sauberer, wenn hier garkein change währe
kziolkowski marked this conversation as resolved
scripts/CSharp/Common/Farming/PlantBehaviour2D.cs Outdated
public override void _Ready()
{
GetTree().CallGroup("PlantGrowing", VesnaAnimations.MethodName.PlayFarmingAnimation);
Jonathan commented 5 months ago
Owner

CallGroup hat ziemlich viel codesmell. Kannst du gerne für jetzt so lassen, aber setz da am besten noch ein To-do hinter, damit wir daran denken, das zu ändern, sobald wir eine event bus implementierung haben.

CallGroup hat ziemlich viel codesmell. Kannst du gerne für jetzt so lassen, aber setz da am besten noch ein To-do hinter, damit wir daran denken, das zu ändern, sobald wir eine event bus implementierung haben.
kziolkowski marked this conversation as resolved
scripts/CSharp/Common/Inventory/ItemResource.cs Outdated
public int maxStack;
[Export]
public PackedScene? itemPrefab;
Jonathan commented 5 months ago
Owner

Ich musste im Code nachschauen, um zu verstehen, wofür das überhaupt ist. Ich denke, das braucht mindestens einen anderen Namen.
Besser aber noch, das kommt in eine neue Klasse oder eine abgeleitete Klasse (z. B. PlantableItemResource)

Außerdem habe ich Angst, da hier ne PackedScene referenziert wird, dass wir wieder irgendwann in ein cyclic dependency problem geraten, das super schwer zu debuggen ist.

Ich musste im Code nachschauen, um zu verstehen, wofür das überhaupt ist. Ich denke, das braucht mindestens einen anderen Namen. Besser aber noch, das kommt in eine neue Klasse oder eine abgeleitete Klasse (z. B. `PlantableItemResource`) Außerdem habe ich Angst, da hier ne PackedScene referenziert wird, dass wir wieder irgendwann in ein cyclic dependency problem geraten, das super schwer zu debuggen ist.
Jonathan commented 5 months ago
Owner

Ich habe noch einmal darüber nachgedacht und bin mir sehr sicher, dass wir damit in eine cyclic dependency geraten, sobald eine Tomatenpflanze nicht nur Tomaten, sondern auch selbst wieder Samen droped. Selbst wenn das Gamedesign technisch nicht vorgesehen ist, schaffen wir uns hier einen ganz gefährlichen Sonderfall.

Wir sollten stattdessen lieber eine Lookup-Table-Resource haben. Darin ist ein Array aus Paaren von ItemResource und PackedScene. Die FieldBehaviour Klasse kann sich basierend auf dem Item das richtige Prefab für die Pflanze raus suchen

Ich habe noch einmal darüber nachgedacht und bin mir sehr sicher, dass wir damit in eine cyclic dependency geraten, sobald eine Tomatenpflanze nicht nur Tomaten, sondern auch selbst wieder Samen droped. Selbst wenn das Gamedesign technisch nicht vorgesehen ist, schaffen wir uns hier einen ganz gefährlichen Sonderfall. Wir sollten stattdessen lieber eine Lookup-Table-Resource haben. Darin ist ein Array aus Paaren von ItemResource und PackedScene. Die FieldBehaviour Klasse kann sich basierend auf dem Item das richtige Prefab für die Pflanze raus suchen
kziolkowski commented 5 months ago
Poster
Owner

Lass mal bei Gelegenheit darüber callen.

Ich bin mir fast sicher, dass deine Bedenken hier unbegründet sind. Der Lösungsvorschlag mit der Lookup-Tabelle ließe sich zwar auch umsetzen, allerdings nicht mit ItemResources als Keys, weil diese nicht von Godot serialisiert werden können und damit in Godot Dictionaries Fehler werfen.

Man kann das umgehen, indem man strings oder enums als identifier benutzt und dazwischenschaltet, das ist aber im Verhältnis zu dieser Lösung sehr viel komplexer und meiner Meinung nach nicht notwendig, da wir ja nicht auf Instanzen direkt zugreifen, sondern nur auf Prefabs / Resourcen verweisen.

Lass mal bei Gelegenheit darüber callen. Ich bin mir fast sicher, dass deine Bedenken hier unbegründet sind. Der Lösungsvorschlag mit der Lookup-Tabelle ließe sich zwar auch umsetzen, allerdings nicht mit ItemResources als Keys, weil diese nicht von Godot serialisiert werden können und damit in Godot Dictionaries Fehler werfen. Man kann das umgehen, indem man strings oder enums als identifier benutzt und dazwischenschaltet, das ist aber im Verhältnis zu dieser Lösung sehr viel komplexer und meiner Meinung nach nicht notwendig, da wir ja nicht auf Instanzen direkt zugreifen, sondern nur auf Prefabs / Resourcen verweisen.
scripts/CSharp/Common/Items/InventoryDependentInteractable.cs
using Babushka.scripts.CSharp.Common.CharacterControls;
Jonathan commented 5 months ago
Owner

Wird das script irgenwo benutzt außer in der InventoryDependentInteractable test scene?

Wird das script irgenwo benutzt außer in der InventoryDependentInteractable test scene?
kziolkowski marked this conversation as resolved
scripts/CSharp/Common/Items/NonInventoryPickup.cs Outdated
{
public void PlayPickupAnimation()
{
GetTree().CallGroup("Pickup", VesnaAnimations.MethodName.PlayPickUpAnimation);
Jonathan commented 5 months ago
Owner

Siehe oben

Siehe oben
kziolkowski marked this conversation as resolved
scripts/CSharp/Common/SceneTransition.cs
{
string sceneName = _sceneNamesToLoad[index];
SceneTransitionThreaded.Instance.ChangeSceneToFile(sceneName);
SceneTransitionThreaded.Instance.ChangeSceneToFileThreaded(sceneName);
Jonathan commented 5 months ago
Owner

Das funktioniert bei mir immer noch nicht.

Bist du dir sicher, dass das bei dir funktioniert? Also nicht nur, wenn du die Farmszene direkt startest, sondern auch wenn du den richtigen Play-Button drückst und aus dem Hauptmenü lädst?

Das funktioniert bei mir immer noch nicht. Bist du dir sicher, dass das bei dir funktioniert? Also nicht nur, wenn du die Farmszene direkt startest, sondern auch wenn du den richtigen Play-Button drückst und aus dem Hauptmenü lädst?
kziolkowski marked this conversation as resolved
kziolkowski added 3 commits 5 months ago
kziolkowski added 6 commits 5 months ago
kziolkowski added 2 commits 4 months ago
kziolkowski added 8 commits 3 months ago
kziolkowski requested review from Jonathan 3 months ago
kziolkowski self-assigned this 3 months ago
Jonathan approved these changes 3 months ago
Jonathan left a comment
Owner

Sieht ok aus.

Gibt noch einige probleme, ich würde das aber so approven, damit es weiter geht.

Sieht ok aus. Gibt noch einige probleme, ich würde das aber so approven, damit es weiter geht.
Jonathan merged commit 40b8c022fe into develop 3 months ago

Reviewers

Jonathan approved these changes 3 months ago
The pull request has been merged as 40b8c022fe.
Sign in to join this conversation.
Reviewers
Request review
No reviewers
Jonathan
Labels
Apply labels
Clear labels
No items
No Label
Milestone
Set milestone
Clear milestone
No items
No Milestone
Assignees
Assign users
Clear assignees
No Assignees
kziolkowski
2 Participants
Notifications
Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Jonathan/Babushka#16
Write Preview
Loading…
Cancel
Save
There is no content yet.
Delete Branch 'feature/showcase_bugfixing_kathi_partII'

Deleting a branch is permanent. It CANNOT be undone. Continue?

No
Yes