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

Biome.SpawnListEntry.newInstance doesn't use entity factories #5046

Closed
bs2609 opened this issue Jul 17, 2018 · 6 comments
Closed

Biome.SpawnListEntry.newInstance doesn't use entity factories #5046

bs2609 opened this issue Jul 17, 2018 · 6 comments

Comments

@bs2609
Copy link
Contributor

bs2609 commented Jul 17, 2018

Currently this just looks for a constructor with a single World parameter and uses that to construct the entity.
It'd be nice if this deferred to EntityEntry.newInstance similarly to EntityList.newEntity, but I don't know if the TODO here would need to be addressed first for this to not be a performance hit:

@Nullable
public static EntityEntry getEntry(Class<? extends Entity> entry)
{
//TODO: Slave map for faster lookup?
for (EntityEntry e : ForgeRegistries.ENTITIES)
{
if (e.getEntityClass() == entry)
return e;
}
return null;
}

@liach
Copy link
Contributor

liach commented Jul 28, 2018

you can just patch spawnlistentry class.

not necessary to adress that. if you go on and add a fast class to entry map, it would be an internal impl detail than a public api. btw vanilla also iterates over all furnace recipes every time to find a match, too. it won't hurt that bad.

@mezz
Copy link
Contributor

mezz commented Jul 28, 2018

I wouldn't want to iterate over the whole registry each time something is spawned, but this is only called on each new SpawnListEntry which is only done a few times at startup so it should be fine.

@bs2609
Copy link
Contributor Author

bs2609 commented Jul 29, 2018

That wouldn't be strictly correct however, as it wouldn't take any subsequent registry changes into account.

@liach
Copy link
Contributor

liach commented Jul 29, 2018

anyways the lookup efficiency is not related to the issue itself. We can easily address the lookup in another pull request

@mezz
Copy link
Contributor

mezz commented Jul 29, 2018

Hmm ok. If you are making it so that SpawnListEntry handles registry changes, then I see what you're saying.
If you are going to be calling getEntry for each spawned entity, I think we definitely need to maintain a lookup map that gets updated along with the registry.

@bs2609
Copy link
Contributor Author

bs2609 commented Jul 29, 2018

As another point, even discounting later registry changes, an eager lookup will cause problems with SpawnListEntry instances created from new Biome(...) calls, as biome registration occurs before entity registration at present.

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

No branches or pull requests

3 participants