Cannot override despawn behavior, forge event called once every 32 ticks

[MrChoke]

Well, one of the main things I want to do with learning forge is see if I can override the default spawning behavior.  Namely how far away a mob is before it despawns for example.  I can't find a way to do it.  And no handling a "LivingSpawnEvent.AllowDespawn" does not do it.  And here is why:

 

Class: net.minecraft.EntityLiving, line 791:

else if ((this.idleTime & 0x1F) == 0x1F && (result = net.minecraftforge.event.ForgeEventFactory.canEntityDespawn(this)) != net.minecraftforge.fml.common.eventhandler.Event.Result.DEFAULT)

 

As you can see the forge event handler is only called every 32nd tick.  I can't modify how long a mob can stay around if all the rest of the ticks will fall into the default behavior!!!!  The placement of this forge event is a failure.  Is there another way to do this?

 

[V0idWa1k3r]

7 hours ago, MrChoke said:

I can't modify how long a mob can stay around if all the rest of the ticks will fall into the default behavior!!!!

I am sorry but what are you talking about? No other ticks will fall into the default behaviour, minecraft simply tries to despawn entities once every 32 ticks and this is where forge hooks into. If this

7 hours ago, MrChoke said:

(this.idleTime & 0x1F) == 0x1F

isn't true no despawning code will be executed at all. And no forge event will fire either because there is no need to.

If you want your mob to stick around for a certain amount of time you can attach a capability to that entity and store your time there.

[MrChoke]

11 hours ago, V0idWa1k3r said:

I am sorry but what are you talking about? No other ticks will fall into the default behaviour, minecraft simply tries to despawn entities once every 32 ticks and this is where forge hooks into. If this

isn't true no despawning code will be executed at all. And no forge event will fire either because there is no need to.

If you want your mob to stick around for a certain amount of time you can attach a capability to that entity and store your time there.

No, I am sorry but you are not correct.  EntityLiving#despawnEntity() is called every tick.  And the code below is such that the default spawn behavior (the ELSE) is called for every tick except the 32nd one and only then is the forge event called:

 

if (this.persistenceRequired)
{
    this.idleTime = 0;
}
else if ((this.idleTime & 0x1F) == 0x1F && (result = net.minecraftforge.event.ForgeEventFactory.canEntityDespawn(this)) != net.minecraftforge.fml.common.eventhandler.Event.Result.DEFAULT)
{
    if (result == net.minecraftforge.fml.common.eventhandler.Event.Result.DENY)
    {
        this.idleTime = 0;
    }
    else
    {
        this.setDead();
    }
}
else
{
    Entity entity = this.world.getClosestPlayerToEntity(this, -1.0D);

    if (entity != null)
    {
        double d0 = entity.posX - this.posX;
        double d1 = entity.posY - this.posY;
        double d2 = entity.posZ - this.posZ;
        double d3 = d0 * d0 + d1 * d1 + d2 * d2;

        if (this.canDespawn() && d3 > 16384.0D)
        {
            this.setDead();
        }

        if (this.idleTime > 600 && this.rand.nextInt(800) == 0 && d3 > 1024.0D && this.canDespawn())
        {
            this.setDead();
        }
        else if (d3 < 1024.0D)
        {
            this.idleTime = 0;
        }
    }
}

 

The forge event is not called enough to be useful in anyway.  In addition, I didn't check the non-forge vanilla minecraft code yet but if Mojang's intent was to have despawning occur every 32nd tick, then Forge created a bug here where despawning can happen ALL but every 32nd tick.

 

Who is Forge?  Is there a group or bug report I can make?  They really need to fix this for 1.13.  

[V0idWa1k3r]

Do you see this line

if (result == net.minecraftforge.fml.common.eventhandler.Event.Result.DENY)
{
	this.idleTime = 0;
}

If the result is deny then the idle time is set to 0 preventing the entity from despawning. From the code you've pasted:

if (this.idleTime > 600 && this.rand.nextInt(800) == 0 && d3 > 1024.0D && this.canDespawn())
{
	this.setDead();
}

The entity will only despawn if idleTime is greater than 600 and denying the event sets it to 0.

[MrChoke]

1 hour ago, V0idWa1k3r said:

Do you see this line

if (result == net.minecraftforge.fml.common.eventhandler.Event.Result.DENY)
{
	this.idleTime = 0;
}

If the result is deny then the idle time is set to 0 preventing the entity from despawning. From the code you've pasted:

if (this.idleTime > 600 && this.rand.nextInt(800) == 0 && d3 > 1024.0D && this.canDespawn())
{
	this.setDead();
}

The entity will only despawn if idleTime is greater than 600 and denying the event sets it to 0.

 

The 1st line you copied, in the forge-event section, called when it returns ALLOW or DENY doesn't matter.  You showing part of the default-behavior ELSE is also missing the point.  The forge-event itself AND the handling after is only called on the 32nd tick.  If we want to override the default behavior with an event handler then we need to handle it on EVERY TICK.

 

Again, this is the line that has the issue:

else if ((this.idleTime & 0x1F) == 0x1F && (result = net.minecraftforge.event.ForgeEventFactory.canEntityDespawn(this)) != net.minecraftforge.fml.common.eventhandler.Event.Result.DEFAULT)

 

You know Java well right?  You understand that ONLY if the below evaluates to TRUE:

(this.idleTime & 0x1F) == 0x1F

 

Will it ever call the forge event handler.  An "&&" doesn't even evaluate the 2nd argument once the 1st is FALSE.  And that's it.  That's all that matters.  The above condition is a logical AND.  It's only true on the 32nd tick of any group of 32 ticks.  

 

Can someone else please comment on this post?  We need another person to deny or agree with the behavior I am saying.  And I still have this question:

If forge itself has a bug (like above), how can I report it?

 

Thanks.

Edited September 15, 2018 by MrChoke

[Draco18s]

22 minutes ago, MrChoke said:

You know Java well right?  You understand that ONLY if the below evaluates to TRUE:

(this.idleTime & 0x1F) == 0x1F

 

Will it ever call the forge event handler.  An "&&" doesn't even evaluate the 2nd argument once the 1st is FALSE.  And that's it.  That's all that matters.  The above condition is a logical AND.  It's only true on the 32nd tick of any group of 32 ticks.  

In order for idleTime to be > 600, idleTime must, at some point, been equal to 32.

 

If it was 32, Forge fired an event, you denied it, idleTime is 0 again.

21 hours ago, MrChoke said:

I can't modify how long a mob can stay around if all the rest of the ticks will fall into the default behavior!

The rest of the ticks are irrelevant unless you're worried about this line:

if (this.canDespawn() && d3 > 16384.0D)

Which if you are, you shouldn't be, because that mob is so far away from the player is freaking irrelevant.

21 hours ago, MrChoke said:

The placement of this forge event is a failure.

Make a pull request, then

 

[MrChoke]

27 minutes ago, Draco18s said:

In order for idleTime to be > 600, idleTime must, at some point, been equal to 32.

 

If it was 32, Forge fired an event, you denied it, idleTime is 0 again.

The rest of the ticks are irrelevant unless you're worried about this line:

if (this.canDespawn() && d3 > 16384.0D)

Which if you are, you shouldn't be, because that mob is so far away from the player is freaking irrelevant.

Make a pull request, then

 

idleTIme can and will increase unless something resets it, like the mob getting attacked for example.  And of course if you return DENY when the event handler is called.  I get that but what if I wanted to modify the behavior coming from the exact line you specify

if (this.canDespawn() && d3 > 16384.0D)

 

That's the the 128 blocks away rule.  If I try to return DENY for a greater distance to keep the mob around, I can't.  As soon ans the default code executes it will despawn.

 

The rule I hate more is this one:

if (this.idleTime > 600 && this.rand.nextInt(800) == 0 && d3 > 1024.0D && this.canDespawn())

 

What if I wanted the despawn check to start after 60 seconds instead of the 30 seconds this line above has?  Again, as soon as I try to extend this with my every 32nd tick, DENY, the default code will despawn him anyway.

 

What is a "PULL REQUEST"?  Can you explain?

 

[Animefan8888]

2 hours ago, MrChoke said:

What is a "PULL REQUEST"?  Can you explain?

 

3 hours ago, MrChoke said:

If forge itself has a bug (like above), how can I report it?

It also requires you give a solution.

[jabelar]

Regarding the "pull request" you can start by just filing an "issue" against the MinecraftForge project on GitHub. The "pull request" is where you actually offer a solution, but honestly it can be a bit tricky to set up properly to contribute to Forge (because Forge is actually a set of patches and because other people are modifying patches as well at the same time you're trying to contribute). However, it is really good if people are able to contribute because ultimately Forge is a community open source project.

 

So I actually responded to someone who had posted this as an issue previously. See https://github.com/MinecraftForge/MinecraftForge/issues/4485

 

In that I looked at the code and here is I summarized the functionality then: Definitely I think the logic could be clearer, but I think the idea is that you can prevent despawning with the event easily because every 32 ticks you get a chance to reset the idleTimer to 0 whereas if you let it get to 600 then there is a chance that closer entities might be despawned. Interestingly though I think the logic is that entities that are very far cannot be prevented from despawning by the event.

 

Now, although the event cannot prevent despawning, the EntityLiving#persistenceRequired field can. That can be set with the EntityLiving#setPersistence() method.

 

So I think that if you want to take total control of despawning you could theoretically set the persistence of any entity you're interested in and then run your own despawning algorithm in the LivingUpdateEvent.

 

Note I do think the despawn event would be better if it was either fired in both code paths or if it was just fired once at the beginning of the method to allow denial of any sort of despawn. I suppose that the original implementers felt that maybe allowing modders to prevent despawn generally would be "dangerous" as there is a real performance impact.

Edited September 16, 2018 by jabelar

[MrChoke]

8 hours ago, jabelar said:

Regarding the "pull request" you can start by just filing an "issue" against the MinecraftForge project on GitHub. The "pull request" is where you actually offer a solution, but honestly it can be a bit tricky to set up properly to contribute to Forge (because Forge is actually a set of patches and because other people are modifying patches as well at the same time you're trying to contribute). However, it is really good if people are able to contribute because ultimately Forge is a community open source project.

 

So I actually responded to someone who had posted this as an issue previously. See https://github.com/MinecraftForge/MinecraftForge/issues/4485

 

In that I looked at the code and here is I summarized the functionality then: Definitely I think the logic could be clearer, but I think the idea is that you can prevent despawning with the event easily because every 32 ticks you get a chance to reset the idleTimer to 0 whereas if you let it get to 600 then there is a chance that closer entities might be despawned. Interestingly though I think the logic is that entities that are very far cannot be prevented from despawning by the event.

 

Now, although the event cannot prevent despawning, the EntityLiving#persistenceRequired field can. That can be set with the EntityLiving#setPersistence() method.

 

So I think that if you want to take total control of despawning you could theoretically set the persistence of any entity you're interested in and then run your own despawning algorithm in the LivingUpdateEvent.

 

Note I do think the despawn event would be better if it was either fired in both code paths or if it was just fired once at the beginning of the method to allow denial of any sort of despawn. I suppose that the original implementers felt that maybe allowing modders to prevent despawn generally would be "dangerous" as there is a real performance impact.

That's for the reply.  YES, that is the exact issue I am reporting here.  I realize that I can modify idleTime to slow or stop despawning (within 128 blocks anyway) but its the distances in the default code that we do not get to control.  

 

So I can re-activate that issue or create a new one but I see in the current one only the original poster and you replied.  Are you on the Forge team?  The fix is easy, simply remove this check:

(this.idleTime & 0x1F) == 0x1F

 

I agree it was done for performance to allow slow complicated despawn handlers to run and not kill the game that much.  But more important is allowing the ability to truly override the default behavior, which it is not doing.  So IMO it's a bug.

 

Anyway I'll create an issue and see what  happens.  Thanks.

[MrChoke]

UPDATE:  So I did a pull request for this issue.  However github would not let me create one for 1.13pre!!  I don't know why.  My pull request says I want to pull 1.13 into 1.12x.  SIgh.

https://github.com/MinecraftForge/MinecraftForge/pull/5148

 

UPDATE 2:  I got git and github desktop installed, forked and pulled the 1.12x Forge codebase.  OK but this does nothing to fix the bug.  What I got is the "net.minecraftforge" source but the bug is in Forge's hook placement in "net.minecraft.entity.EntityLiving".  Modification of this code is required.  How do I get that?

 

Edited September 16, 2018 by MrChoke

[MrChoke]

1 hour ago, diesieben07 said:

If only there was a readme file.

 

I followed that and have the workspace installed on my machine.  Do you?  Do you notice that the only source in it is "net.minecraftforge"?  Where is "net.minecraft" at????

[DaemonUmbra]

From this section of the document:

Quote

The most important thing to note is that if you wish to edit Minecraft source code, you must only do so in the “Forge” sub-project.

 

[MrChoke]

28 minutes ago, DaemonUmbra said:

From this section of the document:

 

Ok, I did read this.  Can you explain what that means?  If I am looking at the pulled code for Forge in my IDE (IntelliJ), where is the "net.minecraft" source?  I am missing this....

 

Thanks

 

[DaemonUmbra]

Did you run the setupForge task?

[MrChoke]

57 minutes ago, diesieben07 said:

Yes, I do. And honestly, I have never navigated to a class manually in the file tree view. You have an IDE, god damn it, use it. "Search for class" is Ctrl-N in IntelliJ.

But, if you must know: Clean sources are in projects/Clean/src/main/java, Forge-Patched sources are in projects/Forge/src/main/java.

I do have an IDE, IntelliJ.  And I did not execute "gradlew setupForge".  Why?  Because the readme you told me to read had that step under the ECLIPSE section, not the IntelliJ section.  Maybe you have been living and breathing Forge and Gradle for years but I haven't.  If the doco is not correct, guess what?  I start asking questions.

 

After running that command with IntelliJ, yes I have the forge sub-project now and it has "net.minecraft" in it.

[MrChoke]

1 hour ago, diesieben07 said:

Actually, the Eclipse documentation is outdated. The IntelliJ section tells you to run the clean and setup tasks. The setup task is the replacement for setupForge.

Well going back, looks like I missed the link to the video for IntelliJ that would have helped.  I need to take a Gradle/Git tutorial or something as well.

 

[MrChoke]

1 hour ago, diesieben07 said:

The video is not needed, everything is explained in the text version as well.

OK... I completely started over.  I realized everything I was doing was off of the 1.12 default branch which I don't want.  I want 1.13.-pre.  I wiped everything related to this off my PC.  I went into GIT and even blew away my repo there as well.

 

I followed the readme line-by-line this time, starting with re-doing the fork to my own repo (and choosing 1.13-pre as the default for it this time).  Then cloning to a brand new folder on my PC.  

I followed the IntelliJ section, yes the text and got to where I opened "build.gradle" as a project.  It started to build and failed.  It didn't get as far as last time when I was going off of 1.12x.  I got this exception when it was trying to configure the project:

 

Caused by: org.gradle.internal.resolve.ModuleVersionNotFoundException: Could not find any version that matches net.minecraftforge.gradle:ForgeGradle:3.+.
 

I attached a  screenshot.

 

I changed nothing.  I pulled whatever was in 1.13-pre branch.

 

 

 

[Animefan8888]

11 minutes ago, MrChoke said:

I pulled whatever was in 1.13-pre branch.

I dont believe 1.13 is in any way ready to be used or have a pr made for it. You need to use 1.12.2

[MrChoke]

3 minutes ago, Animefan8888 said:

I don't believe 1.13 is in any way ready to be used or have a pr made for it. You need to use 1.12.2

Oh.  I guess that makes sense since I was getting much further with 1.12.  I thought since there is a 1.13-pre that people can start submitting changes to it.  I'll go back to 1.12x and go from there.  Thanks.

[MrChoke]

Ok, I finally did this right.  I have a pull request for 1.12 with my change:

 

https://github.com/MinecraftForge/MinecraftForge/pull/5150

 

Does anybody know what comes next?  I assume it needs to get approved?