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

Conditions handled by std::shared_ptr #4829

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MillhioreBT
Copy link
Contributor

  • I have followed [proper The Forgotten Server code styling][code].
  • I have read and understood the [contribution guidelines][cont] before making this PR.
  • I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

The Conditions will simply be handled with std::shared_ptr

Note

Honestly, I have not tried it in the game, although on my local server, which is not 100% TFS, they work fine, however it would be good if you tested it before accepting it.

Issues addressed: Nothing!

Copy link
Contributor

@ramon-bernardo ramon-bernardo left a comment

Choose a reason for hiding this comment

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

Some suggestions

@@ -1177,7 +1177,7 @@ class Player final : public Creature, public Cylinder
std::forward_list<Party*> invitePartyList;
std::forward_list<uint32_t> modalWindows;
std::forward_list<std::string> learnedInstantSpellList;
std::forward_list<Condition*>
std::forward_list<std::shared_ptr<Condition>>
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer using std::shared_ptr<Condition> instead of Condition_ptr. With the explicit type, it's easier to understand 🎉

Copy link
Contributor Author

@MillhioreBT MillhioreBT Nov 15, 2024

Choose a reason for hiding this comment

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

Yes, of course, I am the first person who loves to have a name for everything, however I have fallen into a dilemma, which is why I have uploaded the changes as is, to receive feedback

I like: Condition_ptr but what name can I give to the case of const std::shared_ptr<const Condition>?

It really looks horrible something like this: Condition_ptr_const or Condition_const_ptr

I will leave it up to what the team thinks and of course those people who wish to give good suggestions.

I do not have in mind to abandon the use of const, just for a simple name, I am in favor of making everything possible constness

Copy link
Contributor

Choose a reason for hiding this comment

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

By using std::shared_ptr<T> instead of T_ptr, I figured we wouldn't have Condition_ptr and commented on the clarity, in the end, it doesn't matter

@@ -202,7 +202,7 @@ void BedItem::regeneratePlayer(Player* player) const
{
const uint32_t sleptTime = time(nullptr) - sleepStart;

Condition* condition = player->getCondition(CONDITION_REGENERATION, CONDITIONID_DEFAULT);
std::shared_ptr<Condition> condition = player->getCondition(CONDITION_REGENERATION, CONDITIONID_DEFAULT);
Copy link
Contributor

Choose a reason for hiding this comment

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

auto here?

Copy link
Contributor

Choose a reason for hiding this comment

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

auto here?

image

You are literally contradicting yourself... I don't understand. Either you want the explicit type declared for clarity, or you don't. I understand that "getCondition" would imply that its returning a condition of some sort, raw, ref, shared, we don't know.. but that is implied, which means its implicit... not explicit... so which exactly is your preference?

Copy link
Contributor

Choose a reason for hiding this comment

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

Read it again, until the dot

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.

3 participants