Skip to content

Conversation

@mamadousaliou-bah
Copy link

If there is something wrong, just tell me.

Mamadou Bah added 4 commits June 29, 2024 09:20
…d text color changes based on level difference: green for 0-10 levels, orange for 10-20 levels, and red for differences exceeding 20 levels.
…d, and thresholds for green, yellow, and red kill indicators. Green indicates an acceptable kill, yellow indicates a medium concern, and red indicates a grief kill.
@decaprime
Copy link
Owner

I appreciate the contribution. If you'd like this reviewed for merge please split your changes into focused units without unrelated changes.

Here's issues I found in overview @mamadousaliou-ba, I apologize for not documenting expectations more in a template:

  1. Please provide details in the pull request, how the change was tested, etc. Since visual screenshot or video would be even better to communicate.
  2. Please don't remove the kf label
  3. Was BuffUtil taken from another mod without credit?
  4. Don't check in unneeded changes (e.g. build)
  5. In quick review I don't believe entities don't belong in the statistics model
  6. Please make features optional with so that the mod works for different server scenarios
  7. Review UpdateTopKillersBuff() it's looping over every player (most will be offline) every kill - more deeply: should this keep track of online players instead to do a direct update and also be more dynamic and always in use?
  8. Was an LLM used to generate part of this change? There are oddities like: ```csharp
    UserEntity = Entity.Null, // Default value, update this in your logic where necessary
    CharEntity = Entity.Null // Default value, update this in your logic where necessary
AnsiColor etc.

@mamadousaliou-bah
Copy link
Author

I appreciate the contribution. If you'd like this reviewed for merge please split your changes into focused units without unrelated changes.

Here's issues I found in overview @mamadousaliou-ba, I apologize for not documenting expectations more in a template:

  1. Please provide details in the pull request, how the change was tested, etc. Since visual screenshot or video would be even better to communicate.
  • I will keep that in mind.
  1. Please don't remove the kf label
  • I found PvP to be more stylish, so I changed it that way, but no problem, I can leave it as kf.
  1. Was BuffUtil taken from another mod without credit?
  • BuffUtil comes from KindredCommands. Since it was an open-source project, I didn’t think I needed to specify that it came from there. As you might notice, I am new to open-source and community projects, so some practices are still new to me.
  1. Don't check in unneeded changes (e.g. build)
  • It was an oversight on my part. Copying the .dll directly into the BepInEx folder saved me a lot of time.
  1. In quick review I don't believe entities don't belong in the statistics model
  2. Please make features optional with so that the mod works for different server scenarios
  • I didn't think about that. I made it configurable but not optional yet. I will take that into account.
  1. Review UpdateTopKillersBuff() it's looping over every player (most will be offline) every kill - more deeply: should this keep track of online players instead to do a direct update and also be more dynamic and always in use?
  2. Was an LLM used to generate part of this change? There are oddities like: ```csharp
    UserEntity = Entity.Null, // Default value, update this in your logic where necessary
    CharEntity = Entity.Null // Default value, update this in your logic where necessary
AnsiColor etc.
  • As you might have noticed, mainly by looking at my C# projects, I am new to the C# language and game modding. I have experience with Java and JS, so I have some basics. Generally, what I did was create the necessary code by looking at different repositories and code examples. When I achieved what I wanted, I submitted it to GPT to reformulate/rewrite it as proper C#. This is why you might find certain things like the ones you mentioned in some parts of the code.

Thank you for your patience and guidance. I appreciate your feedback. Please let me know if there are any other areas where I can improve.

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.

2 participants