Update StaticBuilding methods to public about jbwapi HOT 6 OPEN

If BWEM-Community has it public, I vote to match. Not a strongly held opinion.

from jbwapi.

Feel free to make a PR! Otherwise I will look into it when I have time

from jbwapi.

I think we should either leave it private, or make onUnitDestroyed not handle buildings. I'd vote for keeping it as is.

from jbwapi.

I agree with Dan. If it's public in BWAPI we should maintain that across to JBWAPI

Sorry if it was unclear, this is not from (J)BWAPI interface, but the BWEM interface.

The original porters of BWEM to Java decided to add a public method onUnitDestroyed which does onStaticBuildingDestroyed and onMineralDestroyed at the same time (this method does not exist in the reference BWEM-community implementation), making it easier for users of (java) BWEM

For your JBWEB implementation, you probably want to just call onUnitDestroyed(unit), and it will handle it for you as you expect.

There seem to be 4 solutions:

• 1. keeping everything like it is right now
• 2. removing onUnitDestroyed and making the other 2 methods public so the interface is the same as BWEM-community.
• 3. making all 3 of them public
• 4. @Bytekeeper 's second suggestion make onUnitDestroyed not handle buildings

(1) is backwards compatible (maybe some people are already using onUnitDestroyed ) ?
(2) makes the interface the same as how BWEM-community currently is
(3) is also backward compatible, but maybe makes the interface confusing, a new user could be confused by what the difference is between onUnitDestroyed an onMineralDestroyed and maybe assume that BWEM also keeps track of workers or something?
(4) would make onStaticBuildingDestroyed public and keep onUnitDestroyed as a proxy for onMineralDestroyed but potentially silently break some logic of existing bots using onUnitDestroyed, so then I prefer (2), so at least the bot doesnt compile anymore.

I want to release a v2.0 with @dgant 's async support and @Bytekeeper linux support in the near future so I'm not afraid to break some small function calls, especially not if it's about improving/fixing the API .

Personally I favor towards (1) or (2), with a slight tilt towards (1) , as onUnitDestroyed is a nice addition in my opinion, and can just be used for onStaticBuildingDestroyed

I think the problem for most of us is that we dont have a strong opinion on this 😄

@MrTate can you see if just using onUnitDestroyed fulfills your purpose?

from jbwapi.

After going through the code, can you not use: BWMap::onUnitDestroyed ? This method will call the respective onStaticBuildingDestroyed function:

Although the reference BWEM-community implementation has them public and doesnt have this onUnitDestroyed function.
https://github.com/N00byEdge/BWEM-community/blob/9a63141f301f7830e9e09a2bae95c304fcc03cc5/BWEM/include/map.h#L162

I can make both of them public too, but as onUnitDestroy is already there and doing what it should do, maybe it's not needed. Any opinions @MrTate @dgant @Bytekeeper ?

Does the Constructor of StaticBuilding need to be public? Currently onStaticBuildingDestroyed takes a Unit and searches for the corresponding static building internally. If you could share how you'd like to use these changes, Ill make them public, but until then I think it's better to keep them like this.

from jbwapi.

I agree with Dan. If it's public in BWAPI we should maintain that across to JBWAPI

from jbwapi.