Code Lucky Chest event blueprints (without Engine)

[LordPanic]

DISCLAIMER: I know this code is not complete ,  but it's something to begin with for those who want to create this specific event (if they find it useful) saving them some time. It ain't much but it's free.

LuckyBoxNPC.java

LuckyChests.java

NPC XML

 

 

Video

[Kara]

2 hours ago, LordPanic said:

DISCLAIMER: I know this code is not complete ,  but it's something to begin with for those who want to create this specific event (if they find it useful) saving them some time. It ain't much but it's free.

LuckyBoxNPC.java

LuckyChests.java

NPC XML

 

 

Video

 

Ok let's start.

 

1. You're doing an iteration over 

private final List<Npc> _boxList = new ArrayList<>();	

which mean ArrayList<> is not the proper type. Use CopyOnWriteArrayList for thread safety reasons.

 

2. This is completely useless, also you're doing double "setTarget"

if(this.isDead()) 
{
		player.setTarget(player);
		player.setTarget(null);
		return;
}

 (Also this is needless)

 

3. Why initialize a variable using 0 and then add the value again? Why no do it directly?

int chanceToDie = 0;			
chanceToDie = Rnd.get(100);

 

4. Why this check exists in both then and else case?

LuckyChests.getInstance().removeluckyBox(this.getObjectId());

 

[xdem]

1 hour ago, Kara said:

 

Ok let's start.

 

1. You're doing an iteration over 

private final List<Npc> _boxList = new ArrayList<>();	

which mean ArrayList<> is not the proper type. Use CopyOnWriteArrayList for thread safety reasons.

 

lmao why use this CopyOnWriteTrash when his code doesn't seem to have concurrency issues?

ArrayList is the way to go for casual codes like this his list rarely ever gets populated

[Kara]

6 hours ago, xdem said:

lmao why use this CopyOnWriteTrash when his code doesn't seem to have concurrency issues?

ArrayList is the way to go for casual codes like this his list rarely ever gets populated

Didnt you read? Its not completed. Image write this code and remove element while is reading

[melron]

Thanks for sharing. If you wrote this code, could you explain your thought about this line?

doCast(SkillTable.getInstance().getInfo(4143, Math.min(10, Math.round(4143 / 10))));

 

i mean the 2nd parameter of getInfo method. There isn't any dynamic data there and it will always return 10 as level. Whats its purpose?

[LordPanic]

9 hours ago, Kara said:

 

Ok let's start.

 

1. You're doing an iteration over 

private final List<Npc> _boxList = new ArrayList<>();	

which mean ArrayList<> is not the proper type. Use CopyOnWriteArrayList for thread safety reasons.

 

Nope it's fine this is the reason why i use Iterator to begin with. For example this one will lead to CME

public void removeluckyBox(int boxid) {
		for(Npc mob : _boxList) {
			if(mob.getObjectId() == boxid) {							
				_boxList.remove(mob);
				Broadcast.announceToOnlinePlayers("[Lucky Chest]: ["+_boxList.size()+"] left.", true);				
			}			
			//TODO if(_boxList.isEmpty()){ teleport players back and end the event }
		}		
	}

 

Compared to:

public void removeluckyBox(int boxid) {
		for(Iterator<Npc> iterator = _boxList.iterator(); iterator.hasNext();) {
			Npc mob = iterator.next();
			if(mob.getObjectId() == boxid) {							
				iterator.remove();
				Broadcast.announceToOnlinePlayers("[Lucky Chest]: ["+_boxList.size()+"] left.", true);				
			}			
			//TODO if(_boxList.isEmpty()){ teleport players back and end the event }
		}		
	}

 

9 hours ago, Kara said:

 

2. This is completely useless, also you're doing double "setTarget"

if(this.isDead()) 
{
		player.setTarget(player);
		player.setTarget(null);
		return;
}

 

True i guess there is a more effective way to just ignore completely players who target the dead box's. This is the best i could figure out at that time. Im sure there is a packet requestcancelmytarget or whatever to do it the "right" way.

 

9 hours ago, Kara said:

 

 

3. Why initialize a variable using 0 and then add the value again? Why no do it directly?

int chanceToDie = 0;			
chanceToDie = Rnd.get(100);

 

Obviously i could Rnd.get(100) < 50 w.o variables to begin with. I dont even know why i didnt change it.
 

 

9 hours ago, Kara said:

4. Why this check exists in both then and else case?

LuckyChests.getInstance().removeluckyBox(this.getObjectId());

 

True i dont know why i ignored this one.

  

22 minutes ago, melron said:

Thanks for sharing. If you wrote this code, could you explain your thought about this line?

doCast(SkillTable.getInstance().getInfo(4143, Math.min(10, Math.round(4143 / 10))));

 

i mean the 2nd parameter of getInfo method. There isn't any dynamic data there and it will always return 10 as level. Whats its purpose?

I did change it on source forgot to change it to pastebin aswell and yes u are right , just put the 10 which is the lvl of the skill

 

Thanks for  pointing out the mistakes , i will fix them.

Edited September 22, 2021 by LordPanic