The great idea

[br1337]

Why people are using switch statements in sources v140+ and not using array(strategy pattern like), with handlers behind a interface like OdinMS does in v62?

You must be registered to see links

...

 

[kevintjuh93]

Why people are using switch statements in sources v140+ and not using array(strategy pattern like), with handlers behind a interface like OdinMS does in v62?

You must be registered to see links

...

I have no idea. I never knew why they did that, it's so much slower than the strategy pattern.

 

[Straight Edgy]

There's a lot of questionable code written in public Java Emulators, you need to keep in mind there are no qualifications required for developing and releasing a 'repack'/source, which is why you'll find a lot of non-Odin code doesn't keep up to coding conventions.

 

[br1337]

There's a lot of questionable code written in public Java Emulators, you need to keep in mind there are no qualifications required for developing and releasing a 'repack'/source, which is why you'll find a lot of non-Odin code doesn't keep up to coding conventions.

That's true.

I'm trying the source v144 and its a nightmare, haha

I debug things like this:

pse.setLong(++i, iid);
pse.setInt(++i, equip.getUpgradeSlots());
pse.setInt(++i, equip.getLevel());
pse.setInt(++i, equip.getStr());
pse.setInt(++i, equip.getDex());
pse.setInt(++i, equip.getInt());
pse.setInt(++i, equip.getLuk());
pse.setInt(++i, equip.getHp());

 

[SYJourney]

Technically it's a command pattern, because you are not switching out your handlers during runtime. But yeah you're right and using a switch is sort of ugly too.

 

[Novak]

This was extensively discussed before, everybody should know that a linear array is more efficient.

However some would argue that switches are prettier and easier to read (I dissagree), plus you won't notice the difference in performance on a small maplestory server.

My personal conclusion: Someone in the Lithium team was really into switches, (s)he probably just read about them and just had to try em out. In the end, everything based on lithium is sketchy af and if you don't like it you're going to have to spend a lot of time making something different.

 

[SharpAceX]

It also doesn't help that at this point, every "major" source has been touched on by dozens (maybe hundreds) of people, each with varying different skill levels and coding styles. For example, if some beginner added junk to MapleMap years ago, it is probably still floating around today.

 

[ALotOfPosts]

It's because Celino did it in v82 for MSEA and everyone thought it was good because Celino had it. There's a very hivemind thinking process here.
Also doesn't help that decompiled nexon binaries does this.

 

[Snakeday]

It's because Celino did it in v82 for MSEA and everyone thought it was good because Celino had it. There's a very hivemind thinking process here.
Also doesn't help that decompiled nexon binaries does this.

It also makes more sense to do it this way in a language which does static dispatch. For Java it's more or less the same from a performance perpective (both are linear access).
Personally I believe that a huge switch statement is a maintenance nightmare but that performance is good, that's why the oversized array was chosen as the implementation used in OdinMS (as opposed to say a HashMap).

 

[evgz]

It's because Celino did it in v82 for MSEA and everyone thought it was good because Celino had it. There's a very hivemind thinking process here.
Also doesn't help that decompiled nexon binaries does this.

That is not true. It's because RMZero used Celino as base. And there's lots v83+ (excluding v83, more likely v111+) source/repack used RMZero's source as base.
It term of performance, you won't find much differences.
There's also lots bad practices in RMZero's source. I'm not saying he's not a good coder, but lot of his codes are really ugly.

 

[ALotOfPosts]

That is not true. It's because RMZero used Celino as base. And there's lots v83+ (excluding v83, more likely v111+) source/repack used RMZero's source as base.
It term of performance, you won't find much differences.
There's also lots bad practices in RMZero's source. I'm not saying he's not a good coder, but lot of his codes are really ugly.

The difference is between linear and constant. @Snakeday probably meant constant time for hashmap + array. That's quite a big difference, unless you're talking about actual time difference, then I agree it's like a few ns. I agree with the v111+ though. That doesn't mean people coding v111 can't change the code though. The reason they don't is because they think it's proper and people definitely believed it was superior before. Years ago if you even questioned anyone like LightPepsi, XxOsirisxX, Jvlaple, Moogra, you were laughed at. They weren't even that good. You can see all of them were wrong about many things.

 

[Snakeday]

A switch statement gets translated to a jump table and therefore provides constant time access (O(1)). This is probably roughly the same speed (a small bit slower most likely) as the indexed array access and a little faster than a hashmap lookup. Benchmarks are welcome to prove or deny this. All 3 options are O(1)

 

[ALotOfPosts]

I have heard of switch doing that at the compiler level and it might or might not for this case (probably will). This has also been discussed on this forum, something about c doing this too. It's important to note that the compiler won't always do this, and only in certain cases. In this case, it probably will, but definitely not in all cases. I've also heard about the compiler in C doing some binary tree trickery in slightly more spare distributions, but a switch isn't always optimized.

It would still be interesting to see benchmarks though. One with a more completed opcode table, basically any current v83 source. One with a less completed table, like barebones working but with some random packets opcodes added at the middle and the end like 0x1, 0x200, 0x10, 0x12, 0x50, etc. Then finally one with very few. packets added at all. I wonder if the second case won't be optimized if the opcode differences are big enough. The other two should be nearly the same and optimized.

 

[retep998]

Indexing a simple flat array is the fastest option. A switch that optimizes into a jump table in any sane language will often closely match the indexed array in performance, although only if you are switching on an integer. If you switch on strings it is going to be slow and you're gonna have a bad time. Using a hashmap, while still O(1), that constant time factor is significantly slower than the indexed array as it first has to hash the key. The reason to use a Hashmap instead of an indexed array is when you're not working with small integers that are close together, like if you have numbers in the millions or billions with very large gaps in between them that would waste a lot of memory and cause terrible cache performance. A hashmap keeps the values close together so it would be more useful in that case. Also if you're working with a key that is anything other than an integer, a hashmap is the only choice really since you can't index by non-integers.

 

[LightPepsi1]

It's because Celino did it in v82 for MSEA and everyone thought it was good because Celino had it. There's a very hivemind thinking process here.
Also doesn't help that decompiled nexon binaries does this.

I did that for readability. It really doesn't matter, the performance impact is almost zero in modern java VMs. Yes, its a 'hivemind' thinking, but I wouldn't fault anyone for that. There's always a preference for every programmer out there.

I didn't ask anyone to use my source code it just happen that people were basing it off mine.
Here's how it looks ~1.5 years after celino v82 was leaked:

    @Override
    public void messageReceived(final IoSession session, final Object message) throws Exception {
	final SeekableLittleEndianAccessor slea = new GenericSeekableLittleEndianAccessor(new ByteArrayByteStream((byte[]) message));
	final RecvPacketOpcode code = Header.get(slea.readShort());

	if (code != null) {
	    final Client c = (Client) session.getAttribute(StringPool.CLIENT_KEY);

	    if (ServerConstants.enableHandledPacketLogging) {// Debugging
		if (!code.isIgnorePacketSpamOrLog()) {
		    System.out.println(String.format("[%s] %s\nString : %s", code.name(), HexTool.toString((byte[]) message), HexTool.toStringFromAscii((byte[]) message)));
		}
	    }
	    if (code.NeedsChecking()) {
		if (!c.isLoggedIn()) {
		    session.close(true);
		    return;
		}
		/*
		 * if ((code.isCSOperation() && type == ServerType.CHANNEL) ||
		 * (!code.isCSOperation() && type != ServerType.CHANNEL)) {
		 * return; }
		 */
	    }
	    final long cTime = System.currentTimeMillis();
	    final long Differences = cTime - c.LastCapturedTimeMillis_500MSThreshold;
	    if (Differences < 500) { // within 500ms
		if (!code.isIgnorePacketSpamOrLog() && !c.FlagPendingDisconnection) { // Not move life, mob, summon, dragon
		    c.PacketSpamCountWithinHalfSecond++;

		    // 70 should be the acceptable level, but we will test with 200 first to make sure it doesn't affect laggers.
		    if (c.PacketSpamCountWithinHalfSecond > 200) { // Spam > 70 packet within 500ms = dc.
			c.FlagPendingDisconnection();

			ServerLog.RegisterForLogging(ServerLogType.PacketSpam,
				String.format("[%s 0x%s] CharOrAccId : %s, Field : %s, Count : %d\nData : %s\nString : %s",
				code.name(),
				Integer.toHexString(code.getValue()),
				c.getPlayer() != null ? c.getPlayer().getName() : "Accid=" + String.valueOf(c.getAccID()),
				c.getPlayer() != null ? MapleMapFactory.getFullMapName(c.getPlayer().getMapId()) : "-1",
				c.PacketSpamCountWithinHalfSecond,
				HexTool.toString((byte[]) message),
				HexTool.toStringFromAscii((byte[]) message)));
		    }
		}
	    } else {
		c.LastCapturedTimeMillis_500MSThreshold = cTime;
		c.PacketSpamCountWithinHalfSecond = 0;
	    }
	    // This is also used throughout the packet handler to determine the current time instead of calling System.currentTimeMillis() again
	    c.LastCapturedTimeMillis = cTime;
//	    System.out.println("Differences : "+(Differences)+", count : "+c.PacketSpamCountWithinHalfSecond+", cTime : " + cTime);

	    handlePacket(code, slea, c, type);
	} else if (ServerConstants.enableUnhandledPacketLogging) { // Console output part for debugging
	    System.out.println(String.format("[Unhandled Packet] %s\nString : %s", HexTool.toString((byte[]) message), HexTool.toStringFromAscii((byte[]) message)));
	}
    }

    public static final void handlePacket(final RecvPacketOpcode header, final SeekableLittleEndianAccessor slea, final Client c, final ServerType type) {
	switch (header) {
...
}

After all, there are far bigger issues in public source code today such as deadlocks & resource management. (why do people even need that 'unstuck' feature?)
Do note that celino back at may ~ june 2012 before its shut down had over a month of uptime with only 2GB memory usage on its channels.


oh well, its all history anyway maplestory private server is dead, with no one else wanting to contribute.

 

[br1337]

I don't dislike the switch statement, it's just that the way it was implemented is wrong.
We have one class with dozens of handling methods and this kill's the single responsability principle.
In my opinion, code 1 class per each handler is a much clear way of handling packets and it could also be used with switch strategy.

Lets say we want to use our handler in another version of the game.
We really need to import the entire class with the static method handlers? even the stuff that i dont need? I don't think so and that's why I dislike the switch statement and the static classes handling.

 

[SYJourney]

I think that it's important to notice that actually the way most handlers are implemented also violates SRP. This is because most handlers actually have two responsibilities:
- Extract/Parse data from the message
- Decide how to change the world state with that data
The problem with this is that the former has more to do with the app-layer protocol, whereas the latter has something to do with game logic. This is especially true for some of the more complicated handlers, such as the attack handlers. Now of course this still works out alright in practice but if you want to get things 100% right, these two things should be seperated. I'm currently working on this in my own code and I can already see how much cleaner the code becomes after this separation.

 

[br1337]

I think that it's important to notice that actually the way most handlers are implemented also violates SRP. This is because most handlers actually have two responsibilities:
- Extract/Parse data from the message
- Decide how to change the world state with that data
The problem with this is that the former has more to do with the app-layer protocol, whereas the latter has something to do with game logic. This is especially true for some of the more complicated handlers, such as the attack handlers. Now of course this still works out alright in practice but if you want to get things 100% right, these two things should be seperated. I'm currently working on this in my own code and I can already see how much cleaner the code becomes after this separation.

I agree with you.
A good solution would translate the input data in java objects that know nothing about the protocol itself. Those pojos would contain the game logic and wouldn't refer anything about the communication layer in packet handlers(read data/write data directly, ex: writeInt(), readInt(), etc).

So in our game logic layer we would have something more meaniful like: setObjectId() and in communication layer it would be translated into writeInt(objectId);