Update StaticBuilding methods to public #72

Activity

1. JasperGeurtz commented on Oct 29, 2021

   

   JasperGeurtz

   on Oct 29, 2021

   Last edited by JasperGeurtz

   Collaborator

   More actions

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

   👍React with 👍1Reacted by Dan Gant
2. JasperGeurtz commented on Dec 1, 2021

   

   JasperGeurtz

   on Dec 1, 2021

   Collaborator

   More actions

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

   JBWAPI/src/main/java/bwem/BWMap.java

   Line 134 in e5521b2

   onStaticBuildingDestroyed(u);

   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.
3. dgant commented on Dec 3, 2021

   

   dgant

   on Dec 3, 2021

   Contributor

   More actions

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

   👍React with 👍2Reacted by Jasper Geurtz and Tate Hawkersmith
4. Bytekeeper commented on Dec 4, 2021

   

   Bytekeeper

   on Dec 4, 2021

   Last edited by Bytekeeper

   Collaborator

   More actions

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

   👍React with 👍1Reacted by Jasper Geurtz
5. JasperGeurtz

   added

   enhancementNew feature or requestNew feature or request

   on Dec 4, 2021
6. MrTate commented on Dec 5, 2021

   

   MrTate

   on Dec 5, 2021

   Author

   More actions

   I agree with Dan. If it's public in BWAPI we should maintain that across to JBWAPI
7. JasperGeurtz commented on Dec 5, 2021

   

   JasperGeurtz

   on Dec 5, 2021

   Last edited by JasperGeurtz

   Collaborator

   More actions

   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?

   👍React with 👍1Reacted by Dan Gant