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

Fix: QuestGraph #60

Merged

Conversation

alexandrecorlet
Copy link
Contributor

Introduction:
Hello, In this PR, a series of essential corrections and improvements were implemented for the QuestGraph class, providing a more robust and efficient functionality. The main fix focused on the isReachable functionality, which previously did not consider intermediate nodes to reach the destination node. With the Depth-First Search (DFS) strategy now implemented, the method properly considers these nodes, significantly enhancing the accuracy of accessibility checks.

Details of the Fixes:
Before the fix, the isReachable method did not consider the possibility of intermediate nodes to reach the destination node. This could lead to false negatives, indicating that certain nodes were not reachable when they actually were. With the implementation of the Depth-First Search (DFS) strategy, the functionality now properly considers all possible paths to determine the accessibility of the destination node.

Additionally, to ensure the robustness of the fix, additional test cases were created, covering a variety of scenarios to validate the accuracy and reliability of the isReachable method after the adjustment.

Refactorings in the Class:
In addition to the specific fixes, this PR also included a series of refactorings in the QuestGraph class, aiming to improve its readability, maintainability, and overall performance. The most significant of these refactorings occurred in the init() and update() methods, where switches were replaced by a polymorphism-based strategy.

This polymorphic approach allows for more efficient computation of the quest task type and its corresponding treatment, resulting in a considerable reduction in "bad smells" and simplifying the code execution flow.

Conclusion:
In summary, this PR not only addresses a crucial issue in the isReachable functionality of the QuestGraph class but also introduces significant improvements in its internal structure and organization. With these changes, we can ensure a more consistent and reliable experience for users who rely on this class to manage quests in our system.

@hdescottes
Copy link
Owner

Hello, thank you for your contribution and your interest in this project ! :)
I did not look at that quest system in a while and it needed some serious dusting !
I will have a look as quickly as possible. In the meantime, please check the pipeline and try to fix the issues :)

@alexandrecorlet
Copy link
Contributor Author

Hello, thank you for your contribution and your interest in this project ! :) I did not look at that quest system in a while and it needed some serious dusting ! I will have a look as quickly as possible. In the meantime, please check the pipeline and try to fix the issues :)

Sure! I'll review the pipeline and try to fix the issues! :)

Copy link
Owner

@hdescottes hdescottes left a comment

Choose a reason for hiding this comment

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

There is an issue in the quest completion system with what you did.
I can pick up a quest, retrieve teh items needed, but i cannot complete it.

For example :

This PNJ gives you a simple quest, you have to find her baby (in the village)
image
Once you have it, go back to that same PNJ and say "yes", you should have enough XP to level up and this window should appear
image

This does not happen with the current code in your branch :/
(it seems like the test failling on the pipeline is related to that :), once you fix it it should work)

core/src/main/java/com/gdx/game/quest/QuestGraph.java Outdated Show resolved Hide resolved
core/src/main/java/com/gdx/game/quest/QuestGraph.java Outdated Show resolved Hide resolved
@alexandrecorlet
Copy link
Contributor Author

alexandrecorlet commented May 18, 2024

There is an issue in the quest completion system with what you did. I can pick up a quest, retrieve teh items needed, but i cannot complete it.

For example :

This PNJ gives you a simple quest, you have to find her baby (in the village) image Once you have it, go back to that same PNJ and say "yes", you should have enough XP to level up and this window should appear image

This does not happen with the current code in your branch :/ (it seems like the test failling on the pipeline is related to that :), once you fix it it should work)

Hi,

Apparently, the problem occurred because I've accidentally changed the behavior of the methods updateQuestForReturn and isQuestTaskAvailable when I refactored them. I've fixed the issue by reverting both methods to their previous implementation. The game seems to be working now. :)

@hdescottes
Copy link
Owner

There is an issue in the quest completion system with what you did. I can pick up a quest, retrieve teh items needed, but i cannot complete it.
For example :
This PNJ gives you a simple quest, you have to find her baby (in the village) image Once you have it, go back to that same PNJ and say "yes", you should have enough XP to level up and this window should appear image
This does not happen with the current code in your branch :/ (it seems like the test failling on the pipeline is related to that :), once you fix it it should work)

Hi,

Apparently, the problem was because I've accidentally changed the behavior of the methods updateQuestForReturn and isQuestTaskAvailable when I refactored them. I've fixed the issue by reverting both methods to their previous implementation. It seems that game is working properly now. :)

nice i will review that asap :)

Copy link
Owner

@hdescottes hdescottes left a comment

Choose a reason for hiding this comment

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

The quest completion system is fine now :)

But i have spotted something weird. NPCs should always have some random conversation like "Hi", "Greetings stranger", etc... if they don't have quests for the player. This does not work anymore :/ (2db9e19)
Fix this and make sure to write a test about this behaviour in order to not have this error again.

@alexandrecorlet
Copy link
Contributor Author

alexandrecorlet commented May 19, 2024

The quest completion system is fine now :)

But i have spotted something weird. NPCs should always have some random conversation like "Hi", "Greetings stranger", etc... if they don't have quests for the player. This does not work anymore :/ (2db9e19) Fix this and make sure to write a test about this behaviour in order to not have this error again.

Hi, Hugo. How are you? I'm glad the quest completion system is working fine now! :)

Did you spot this strange behavior on the master or fix/questGraph branch? I've tried to reproduce the problem you mentioned on the fix/questGraph, and the NPC behaved normally:

Screen Shot 2024-05-19 at 14 59 32

I've also checked the game logs, and they're ok too:

Screen Shot 2024-05-19 at 14 59 39

So, after investigating a little bit, I've switched to the master branch, and tried to reproduce the bug you mentioned there. This is the game log when I tried to speak with a NPC on the master branch:

Screen Shot 2024-05-19 at 14 58 05

The bug is occurring because the conversation file does not exist on the master branch.

@hdescottes
Copy link
Owner

hdescottes commented May 19, 2024

Huh, that's weird, because i can see the file in both masters branch (mine and yours).
But i can't see it when i copy your repo with your branch
WEIRD

I will merge the PR as it seems that it is something that somehow i did while copy your repo.

Congratulations ! :)

@hdescottes hdescottes merged commit c30bf9f into hdescottes:master May 19, 2024
1 check passed
@alexandrecorlet
Copy link
Contributor Author

alexandrecorlet commented May 19, 2024

Yeah, IDK how this happened lol. REALLY WEIRD ;-;

Anyway, I'm glad I could contribute to your project! Thanks for the opportunity and hopefully we can work together again sometime! Thx again and happy coding, friend! :)

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.

None yet

2 participants