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: add new message types and handlers, refactor client and server code #2264

Open
wants to merge 33 commits into
base: main
Choose a base branch
from

Conversation

Pospelove
Copy link
Contributor

@Pospelove Pospelove commented Dec 11, 2024

closes #2267

Important

Add new message types and handlers, refactor client and server code to improve modularity and support new functionality.

  • New Message Types:
    • Added ActivateMessage, ConsoleCommandMessage, CraftItemMessage, CustomEventMessage, DestroyActorMessage, DropItemMessage, FinishSpSnippetMessage, HitMessage, HostMessage in skymp5-server/cpp/messages.
    • Updated MsgType.h to include new message types.
  • Client Changes:
    • Refactored gamemodeEventSourceService.ts, remoteServer.ts, sendInputsService.ts, spSnippetService.ts to handle new message types.
    • Renamed changeValues.ts to changeValuesMessage.ts and updated imports in events.ts, anyMessage.ts, deathStateContainerMessage.ts, remoteServer.ts, sendInputsService.ts.
    • Updated consoleCommandMessage.ts, customEventMessage.ts, deathStateContainerMessage.ts, finishSpSnippetMessage.ts to reflect new message structures.
  • Server Changes:
    • Implemented new message handlers in PacketParser.cpp for Activate, ConsoleCommand, CraftItem, CustomEvent, DropItem, FinishSpSnippet, OnHit, Host.
    • Updated ConsoleCommands.cpp and ConsoleCommands.h to support new argument types and commands.
  • Miscellaneous:
    • Refactored clang-format-hook.js to support new file checks and formatting.

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

@ellipsis-dev ellipsis-dev bot changed the title . feat: add new message types and handlers, refactor client and server code Dec 11, 2024
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 bdb89bc in 5 minutes and 55 seconds

More details
  • Looked at 817 lines of code in 26 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. skymp5-client/src/services/messages/consoleCommandMessage.ts:10
  • Draft comment:
    The args property in ConsoleCommandMessage is defined as Array<number | string> in TypeScript, but in C++ it's defined as std::vector<std::variant<int64_t, std::string>>. This inconsistency could lead to issues when handling the data across the two languages. Consider ensuring consistent data types across both implementations.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. skymp5-client/src/services/messages/customEventMessage.ts:5
  • Draft comment:
    The argsJsonDumps in CustomEventMessage is defined as string[] in TypeScript, but in C++ it's defined as std::vector<std::string>. Ensure that the serialization and deserialization processes handle these consistently across both languages.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The argsJsonDumps in CustomEventMessage is defined as string[] in TypeScript, but in C++ it's defined as std::vector<std::string>. While these are similar, it's important to ensure that the serialization and deserialization processes handle these consistently across both languages.
3. skymp5-server/cpp/messages/ConsoleCommandMessage.h:21
  • Draft comment:
    The args property in ConsoleCommandMessage is defined as std::vector<std::variant<int64_t, std::string>>, but in TypeScript, it's Array<number | string>. Ensure consistent data types across both implementations to avoid potential issues.
  • Reason this comment was not posted:
    Marked as duplicate.
4. skymp5-server/cpp/messages/FinishSpSnippetMessage.h:20
  • Draft comment:
    The returnValue in FinishSpSnippetMessage is defined as std::optional<std::variant<bool, double, std::string>>, but in TypeScript, it's boolean | number | string. Consider aligning the types to avoid potential precision issues.
  • Reason this comment was not posted:
    Marked as duplicate.
5. skymp5-server/cpp/messages/CustomEventMessage.h:15
  • Draft comment:
    The argsJsonDumps in CustomEventMessage is defined as std::vector<std::string>, but in TypeScript, it's string[]. Ensure that the serialization and deserialization processes handle these consistently across both languages.
  • Reason this comment was not posted:
    Confidence changes required: 50%
    The argsJsonDumps in CustomEventMessage is defined as std::vector<std::string> in C++, but in TypeScript, it's string[]. While these are similar, it's important to ensure that the serialization and deserialization processes handle these consistently across both languages.

Workflow ID: wflow_o783WrhA7Fkllw9l


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.

@@ -2,6 +2,6 @@ import { MsgType } from "../../messages"

export interface FinishSpSnippetMessage {
t: MsgType.FinishSpSnippet,
returnValue: unknown, // TODO: improve type: there should union of possible Papyrus values
returnValue?: boolean | number | string;
Copy link

Choose a reason for hiding this comment

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

The returnValue in FinishSpSnippetMessage is defined as boolean | number | string in TypeScript, but in C++ it's defined as std::optional<std::variant<bool, double, std::string>>. The use of double in C++ might lead to precision issues if the number is an integer in TypeScript. Consider aligning the types to avoid potential issues.

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.

CMake: 'codegen' is a reserved target name
2 participants