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

OSRS Integration #473

Open
wants to merge 63 commits into
base: kotlin-experiments
Choose a base branch
from

Conversation

Khaled431
Copy link

Start of the potential move to OSRS.

@garyttierney
Copy link
Contributor

Related to #472.

Khaled Jaber added 6 commits February 29, 2020 16:13
Renamed XteaDecoder.java to XteaParser.java
Added XteaUtil which decrypts/encrypts a ByteBuf with Xtea rounds.
Added UserStats which details the incoming connection's hardware specs.
Added user stats to the credentials to replace uid.
Resolved issue with Crc check being invalid due to index sixteen being manually zeroed over the wire by Jagex.
@Khaled431 Khaled431 changed the title Cache definitions, update server and most of login done for rev 181. OSRS Integration Mar 1, 2020
@Major- Major- added the osrs label Mar 1, 2020
Khaled Jaber added 20 commits March 1, 2020 18:08
… someone decides how to move forward with a multi-world impl.
… someone decides how to move forward with a multi-world impl.
Added the following decode messages: Add/Remove Ignores&Friends, Private Chat Messages.
Report abuse decoder.
Keepalive decoder.
Walk decoder.
Arrowkey decoder.
Close interface decoder.
Amount input decoder.

Removed 317 rev and 377 to follow soon.
Note: You may need to exclude the 377 directory manually.
Object options decoders.
… organized but if anyone has suggestions I will gladly listen.
… organized but if anyone has suggestions I will gladly listen.
…ws-1252 characters.

More decoding messages, have these remaining so far:

ButtonMessageDecoder
DialogueContinueMessageDecoder
FlaggedMouseEventMessageDecoder

ItemOnItemMessageDecoder
ItemOnNpcMessageDecoder
ItemOnObjectMessageDecoder

SwitchItemMessageDecoder
TakeTileItemMessageDecoder

MagicOnItemMessageDecoder
MagicOnNpcMessageDecoder
MagicOnPlayerMessageDecoder

FirstItemOptionMessageDecoder
SecondItemOptionMessageDecoder
ThirdItemOptionMessageDecoder
FourthItemOptionMessageDecoder
FifthItemOptionMessageDecoder

PlayerDesignMessageDecoder
PublicChatMessageDecoder

Doesn't exist anymore:
FlashingTabClickedMessageDecoder
FirstItemActionMessageDecoder
SecondItemActionMessageDecoder
ThirdItemActionMessageDecoder
FourthItemActionMessageDecoder
FifthItemActionMessageDecoder
NPCS are complete w/o masks.
EnumDefinition.java support for OSRS Enums.
DisplayStatusMessage has been supported (The ability to switch the display mode).
Standard named for id << component should be packedInterface.
Added IfSetEventMessage encoder.
Added KeepAliveMessageDecoder message.
Copy link
Contributor

@garyttierney garyttierney left a comment

Choose a reason for hiding this comment

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

This is a first pass to get rid of the code churn in this PR so I can review the actual code changes. We need to get rid of the commit that deletes the Ruby and the commits that optimize imports and I think this will be easier to review.

@@ -5,6 +5,10 @@ description = 'Apollo Cache'
dependencies {
implementation project(':util')
implementation group: 'com.google.guava', name: 'guava', version: guavaVersion
implementation group: 'org.tukaani', name: 'xz', version: '1.8'
implementation group: 'it.unimi.dsi', name: 'fastutil', version: '8.3.1'
implementation group: 'org.apache.ant', name: 'ant', version: '1.10.7'
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we depending on ant?

Copy link
Author

Choose a reason for hiding this comment

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

I forget.

Copy link
Member

Choose a reason for hiding this comment

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

So... can it be removed?

implementation group: 'org.tukaani', name: 'xz', version: '1.8'
implementation group: 'it.unimi.dsi', name: 'fastutil', version: '8.3.1'
implementation group: 'org.apache.ant', name: 'ant', version: '1.10.7'
implementation group: 'com.google.code.gson', name: 'gson', version: '2.8.6'
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd recommend using Jackson instead of Gson.

Copy link
Author

Choose a reason for hiding this comment

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

Sure, no difference to me.


import org.apollo.cache.IndexedFileSystem;
import org.apollo.cache.archive.Archive;
import it.unimi.dsi.fastutil.ints.Int2ObjectArrayMap;
Copy link
Contributor

Choose a reason for hiding this comment

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

Use standard collections instead of these fastutil collections. We optimize on a case by case basis, not generally like this.

Copy link
Author

Choose a reason for hiding this comment

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

That's fine. I'll get rid of it.

definition.setF2117(buffer.readUShort());
definition.setF2118(buffer.readUByte());
} else if (opcode == 79) {
definition.setF2119(buffer.readUShort());
Copy link
Contributor

Choose a reason for hiding this comment

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

If a field is unknown we don't assign it to unused fields, instead we just skip the data.

Copy link
Author

Choose a reason for hiding this comment

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

To be faaaaaaaiiiiiiir I c&p this out of my cache editor. But i'll get rid of them.

@@ -1,26 +1,41 @@
<messages>
<message>
<type>org.apollo.game.message.impl.ButtonMessage</type>
<type>org.apollo.game.message.impl.decode.IfActionMessage</type>
Copy link
Contributor

Choose a reason for hiding this comment

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

These class names aren't descriptive. They need to have meaningful names without abbreviations.

Copy link
Author

@Khaled431 Khaled431 Apr 25, 2020

Choose a reason for hiding this comment

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

That's the actual name Jagex use., I prefer it but I can see how it's not descriptive. I'll change it back.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's the actual name Jagex use

This is a fairly tired argument. A lot of what Jagex done is garbage.

Copy link
Author

Choose a reason for hiding this comment

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

I'm not disagreeing, I'm just explaining the decision.

import java.util.concurrent.Executors;
import java.util.concurrent.Future;
import java.util.concurrent.TimeUnit;
import java.util.concurrent.*;
Copy link
Contributor

Choose a reason for hiding this comment

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

You've ran IntelliJ's "optimize imports" function over the code, this isn't the place to be doing that.

Copy link
Author

Choose a reason for hiding this comment

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

I have it set to auto do it. Sorry.

if (player.getInterfaceSet().contains(BankConstants.BANK_WINDOW_ID)) {
if (message.getInterfaceId() == BankConstants.SIDEBAR_INVENTORY_ID) {
if (player.getInterfaceSet().contains(BankConstants.WINDOW_ID)) {
//if (message.getInterfaceId() == BankConstants.SIDEBAR_INVENTORY_ID) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Odd code here. Why is this commented out?

Copy link
Author

Choose a reason for hiding this comment

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

Honestly I don't remember. All interfaces have to be redone regardless.

Copy link
Author

Choose a reason for hiding this comment

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

@garyttierney It's because the interface doesn't exist anymore. I've removed it entirely now.

world.writeByte(0); // country
world.writeShort(2000);// number of players
return world;
private ByteBuffer createWorld() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we actually need this to get login working?

Copy link
Author

Choose a reason for hiding this comment

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

Client freezes without it when clicked on by mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, then I'd be inclined to make it return real data. cc @Major-

/**
* The type Op item message.
*/
public class OpItemMessage extends Message {
Copy link
Contributor

Choose a reason for hiding this comment

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

Naming needs reverted.

Copy link
Author

Choose a reason for hiding this comment

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

Reverted to what exactly? Apollo never had ground items.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then pick something sensible that isn't nonsense like OpItem.

*
* @param buffer the buffer
*/
public UserStats(ByteBuf buffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

A constructor isn't the place to be reading from a byte buffer. Create a static constructor and make it clear that the buffer is consumed.

util/src/main/java/org/apollo/util/BufferUtil.java Outdated Show resolved Hide resolved
@@ -724,7 +735,8 @@ public void send(Message message) {
* Sends the initial messages.
*/
public void sendInitialMessages(DisplayMode mode) {
send(new RebuildNormalMessage(position, index, world.getRegionRepository().getXteaRepository(), false));
send(new RebuildNormalMessage(position, index, updateInfo, world.getPlayerRepository(),
Copy link
Contributor

Choose a reason for hiding this comment

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

This is an odd way to pass data for initialization, cc @Major-, we need to look at setting up player "pre-init" code.

Copy link
Author

Choose a reason for hiding this comment

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

I wasn't sure how to pass it around. You could do everything with a single player reference but I tried to keep it consistent with how other things are done.

*/
private static final int MAXIMUM_LOCAL_PLAYERS = 255;
private void prefetch() {
limitedPlayers.clear();
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is an approximation of:

for (Player updatingPlayer : playersToUpdate) {
   for (Region region : regions) {
      for (Player candidate: regions.getPlayers()) {
         // test
      }
   }
}

is that right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm, this is what we do currently. That doesn't quite scale. @Major- @rmcmk ever thought about this? Scanning close regions or adding to this change buffer with an event system seems like a better idea.

Copy link
Author

Choose a reason for hiding this comment

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

That's not exactly correct.

for (Region region : regions) {
for (Player candidate: regions.getPlayers()) {
// test
}
}

Is more what it is like in that function. If you're referring to what it does for every player combined,, then yes.

@@ -5,6 +5,10 @@ description = 'Apollo Cache'
dependencies {
implementation project(':util')
implementation group: 'com.google.guava', name: 'guava', version: guavaVersion
implementation group: 'org.tukaani', name: 'xz', version: '1.8'
implementation group: 'it.unimi.dsi', name: 'fastutil', version: '8.3.1'
implementation group: 'org.apache.ant', name: 'ant', version: '1.10.7'
Copy link
Member

Choose a reason for hiding this comment

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

So... can it be removed?

@softagram
Copy link

softagram bot commented May 13, 2020

Softagram Impact Report for pull/473 (head commit: ba35a57)

⚠️ Copy paste found (largest 3 shown)

ℹ️ ItemOnObjectVerificationHandlerTests.java: Copy paste fragment inside the same file on lines 47, 79:


  World world = mock(World.class);
  Region region = mock(Region.class);
  RegionRepository regionRepository = mock(RegionRepository.class);
  Pl...(truncated 788 chars)

ℹ️ ItemOnObjectVerificationHandlerTests.java: Copy paste fragment inside the same file on lines 56, 121:


  when(player.getInventory()).thenReturn(inventory);
  when(world.getRegionRepository()).thenReturn(regionRepository);
  when(regionRepository.fromPosition(...(truncated 733 chars)

ℹ️ ObjectActionVerificationHandlerTests.java: Copy paste fragment inside the same file on lines 49, 75:


  when(world.getRegionRepository()).thenReturn(regionRepository);
  when(regionRepository.fromPosition(objectPosition)).thenR...(truncated 598 chars)

Now that you are on the file, it would be easier to pay back some tech. debt.

⭐ Change Overview

Showing the changed files, dependency changes and the impact - click for full size
(Open in Softagram Desktop for full details)

⭐ Details of Dependency Changes

details of dependency changes - click for full size
(Open in Softagram Desktop for full details)

📄 Full report

Impact Report explained. Give feedback on this report to [email protected]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants