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

effect manager #64

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

pythoncoder1234
Copy link
Contributor

@pythoncoder1234 pythoncoder1234 commented Jan 22, 2025

  • installed new library: fastutil
    • faster versions of java collections
  • effect manager to manage attributes and status effects
  • optimized effect managing using hashmaps and
  • get/set attribute/status effect methods have been moved to effect manager
    • call movable.em().function() to access
  • most of these changes were from refactoring the above, i promise there aren't that many changes

Signed-off-by: pythoncoder1234 <[email protected]>
Signed-off-by: pythoncoder1234 <[email protected]>
@ghostlypi
Copy link
Collaborator

Just an FYI, TanksJson got killed because I was trying to use GSON instead of @aehmttw's own TanksON library. I don't think @aehmttw likes adding new libs for something as small as a few percent performance boost.

@pythoncoder1234
Copy link
Contributor Author

I think he kinda wanted to use the one he wrote in cs class... we'll see. Also the performance boost is not just a few percent, especially since the collections avoid unboxing where possible it saves a ton of memory and improves performance.

@aehmttw
Copy link
Owner

aehmttw commented Jan 22, 2025

I got it to crash in arcade mode multiplayer

java.lang.NullPointerException: Cannot invoke "tanks.BiConsumer.accept(Object, Object)" because "this.addAttributeCallback" is null at tanks.effect.EffectManager.addUnduplicateAttribute(EffectManager.java:166) at tanks.effect.EffectManager.addStatusEffect(EffectManager.java:223) at tanks.effect.EffectManager.addStatusEffect(EffectManager.java:183) at tanks.AreaEffectFreeze.update(AreaEffectFreeze.java:92) at tanks.gui.screen.ScreenGame.update(ScreenGame.java:1543) at tanks.Panel.update(Panel.java:569) at tanks.GameUpdater.update(GameUpdater.java:23) at lwjglwindow.LWJGLWindow.tick(LWJGLWindow.java:399) at lwjglwindow.LWJGLWindow.loop(LWJGLWindow.java:330) at lwjglwindow.LWJGLWindow.run(LWJGLWindow.java:123) at main.Tanks.main(Tanks.java:106)

Signed-off-by: pythoncoder1234 <[email protected]>
with:
lfs: true

- name: Cache Gradle
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the Cache Gradle action for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's supposed to cache the gradle distribution... not 100% sure it works yet. It seems to be building faster but I'm not sure.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think gradle caching does anything. That is because github actions start each action in a new container, so we compile and cache with gradle, but never revisit the cache.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

my codeql builds run in less than a minute, faster than runs on the master branch here which take 2-3m. I seem to be doing something right...

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This action took 45 s to build, and it does not have caching enabled. I think that it might be a bit random tbh.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it’s probably proportional to the number of classes changed then

@aehmttw
Copy link
Owner

aehmttw commented Jan 23, 2025

What is the advantage of this system over the old one?

@pythoncoder1234
Copy link
Contributor Author

  • adds StatusEffect modifiers to the attributes list directly (instead of additionally having to iterate through all status effects and their modifiers)
    • better implementation in my opinion, it shouldn't make the code much more complicated
  • uses more hashmaps to improve performance (mapped by name -> modifier, type -> ArrayList)
  • optimized memory usage and performance using faster data structures from fastutil
    • example: iterating through attributes.values() once per tank per frame is a memory disaster, has been fixed
  • split movable and effect-related stuff into different files, in a way that imo is intuitive

@ghostlypi
Copy link
Collaborator

I don't think your changes to the Compatibility class allow the class to do what it is designed to do. What you have done is generalized the function to convert a boolean into a TanksONable, however, you failed to consider other cases which may require more specific special cases. An example would be to take an integer value, and convert it to an object where the integer value may be a single value within the new object. Additionally, you outright remove the code that would enable different fields to be renamed and have their values carried over between game versions. This compatibility subsystem needs to be "future proof" to some degree and be able to expand over time. If you disagree with the design of a system, you should propose an alternative design first so as to not accidentally remove functionality from an existing system.

@aehmttw
Copy link
Owner

aehmttw commented Jan 24, 2025

This pull request does not have anything to do with compatibility so I don't know why that's included here. If you wish to PR compatibility, please make a separate pull request. Thanks.

@pythoncoder1234
Copy link
Contributor Author

whoops reverted it

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

Successfully merging this pull request may close these issues.

3 participants