Skip to content

Conversation

Celant
Copy link
Collaborator

@Celant Celant commented Jul 8, 2017

Mass updated most packets to bring them in-line with the latest Terraria version.

Copy link

@Ijwu Ijwu left a comment

Choose a reason for hiding this comment

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

I'm a jerk and I dislike unnecessary colons. There are other items that should be thought about as well.

public override string ToString()
{
return string.Format("[CompleteConnectionandSpawn]");
return $"[CompleteConnectionandSpawn:]";
Copy link

Choose a reason for hiding this comment

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

What's going on here? You like colons, georg?

public override string ToString()
{
return string.Format("[ContinueConnecting2]");
return $"[ContinueConnecting2:]";
Copy link

Choose a reason for hiding this comment

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

your colons allude to further content, but that content does not exist.

did you auto-gen these classes? lol

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@Ijwu Yes, yes I did. I used MultiSimplicity

public override string ToString()
{
return $"[CrystalInvaionWipeAll]";
return $"[CrystalInvasionWipeAll:]";
Copy link

Choose a reason for hiding this comment

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

I know, I'm a pedantic asshole

public override string ToString()
{
return string.Format("[Deprecated]");
return $"[Deprecated:]";
Copy link

Choose a reason for hiding this comment

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

I'm just the worst

/// <summary>
/// Gets or sets the Var1 - KillTile (Fail: Bool), PlaceTile (Type: Byte), KillWall (Fail: Bool), PlaceWall (Type: Byte), KillTileNoItem (Fail: Bool), SlopeTile (Slope: Byte)|
/// </summary>
public short Var1 { get; set; }
Copy link

Choose a reason for hiding this comment

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

This and Var2 could be more aptly named. (Like they were before, haha)

public override string ToString()
{
return string.Format("[SocialHandshake]");
return $"[SocialHandshake:]";
Copy link

Choose a reason for hiding this comment

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

:

public override string ToString()
{
return string.Format("[TeleportationPotion]");
return $"[TeleportationPotion:]";
Copy link

Choose a reason for hiding this comment

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

:

public override string ToString()
{
return string.Format("[ToggleBirthdayParty]");
return $"[ToggleBirthdayParty:]";
Copy link

Choose a reason for hiding this comment

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

the one colon to rule them all

namespace Multiplicity.Packets
{
[Flags]
public enum PlayerControlFlags : byte
Copy link

Choose a reason for hiding this comment

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

The removal of these flags makes it somewhat harder to determine which controls a player is pressing. I'd rather use the built-in flag comparisons in C# rather than the bitflag checking extensions.

Plus, these flags add some semantics to the bit places. I think they should be returned and utilized.

/// <summary>
/// Gets or sets the EventInfo2 - BitFlags: 1 = Mech Boss Downed, 2 = Mech Boss Downed 2, 4 = Mech Boss Downed 3, 8 = Mech Boss Any Downed, 16 = Cloud BG, 32 = Crimson, 64 = Pumpkin Moon, 128 = Snow Moon|
/// </summary>
public byte EventInfo2 { get; set; }
Copy link

Choose a reason for hiding this comment

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

Hmm, I mentioned using the bitflags in my previous comment about the player controls. It could be useful to have a Flags enum for things like this as well. This isn't strictly relevant to this PR, but this could be added later on. Let's keep it in mind.

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