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

feat(skymp5-server): implement rest of Utility functions #2323

Conversation

Wedmer
Copy link
Contributor

@Wedmer Wedmer commented Feb 9, 2025

Important

Implement additional utility functions in PapyrusUtility for waiting, game time, and array operations, with helper methods for refactoring.

  • Behavior:
    • Refactor Wait() in PapyrusUtility.cpp to use WaitHelper().
    • Add WaitMenuMode(), WaitGameTime(), IsInMenuMode(), GetCurrentGameTime(), GameTimeToString() to PapyrusUtility.cpp.
    • Add array creation and resizing functions: CreateAliasArray(), CreateBoolArray(), CreateFloatArray(), CreateFormArray(), CreateIntArray(), ResizeAliasArray(), ResizeBoolArray(), ResizeFloatArray(), ResizeFormArray(), ResizeIntArray().
  • Helpers:
    • Introduce WaitHelper() and ArrayHelper() in PapyrusUtility.cpp for handling wait and array operations.
  • Registration:
    • Register new functions in Register() method in PapyrusUtility.cpp.

This description was created by Ellipsis for c85bbad. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

❌ Changes requested. Reviewed everything up to c85bbad in 2 minutes and 34 seconds

More details
  • Looked at 267 lines of code in 2 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:203
  • Draft comment:
    Capturing the promise by value in the lambda may break resolution. Consider capturing by reference or using move semantics.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
    Promise objects often need careful handling with regards to ownership and lifetime. However, without seeing the implementation of Promise, VarValue, and how they handle copying/moving, it's hard to be certain if value capture is problematic here. The code was just moved to a helper function but the core logic remains unchanged.
    I don't have visibility into the Promise implementation to know if value capture is actually problematic. The code may work perfectly fine as is.
    Without clear evidence that value capture is causing issues, and without being able to see the Promise implementation, this comment is too speculative.
    Delete the comment since we don't have strong evidence that value capture is problematic, and the suggestion is speculative without understanding the Promise implementation details.
2. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.h:10
  • Draft comment:
    Fix typos in parameter names ('slef' should be 'self').
  • Reason this comment was not posted:
    Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:203
  • Draft comment:
    Lambda in WaitHelper captures resultPromise by value and calls non-const methods (Resolve/Reject). Mark the lambda as mutable or capture resultPromise by reference to ensure these calls compile.
  • Reason this comment was not posted:
    Marked as duplicate.
4. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:215
  • Draft comment:
    The argument count check in ArrayHelper is incorrect. For non-resize calls, two arguments are required (size and fill value), and for resize calls, three arguments are needed. Update the condition accordingly.
  • Reason this comment was not posted:
    Marked as duplicate.
5. skymp5-server/cpp/server_guest_lib/script_classes/PapyrusUtility.cpp:219
  • Draft comment:
    Consider using static_cast<size_t> for arraySize instead of static_cast<int32_t> to match the variable type and avoid potential sign issues.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None

Workflow ID: wflow_JfpymfZrppQoYoK0


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Ensure we have all arguments.
Ensure that array size is not negative number.

Signed-off-by: Vladimir Ulrich <[email protected]>
Signed-off-by: Vladimir Ulrich <[email protected]>
@Wedmer
Copy link
Contributor Author

Wedmer commented Feb 9, 2025

Implementation for INI, Budget, Memory functions wasn't added as I can't imagine any use of them.

@nic11
Copy link
Collaborator

nic11 commented Feb 9, 2025

Let's see it on the test server

@Wedmer Wedmer requested a review from Pospelove February 9, 2025 21:46
@Pospelove
Copy link
Contributor

Looks safe and correct. We can merge it after morning update I guess

@Pospelove Pospelove merged commit 8ac4233 into skyrim-multiplayer:main Feb 10, 2025
12 of 13 checks passed
nic11 added a commit to nic11/skymp that referenced this pull request Feb 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants