I am programming a game for years now but I have a question about how I programmed things up.
So imagine you have a gun class for a game, guns can have many sprites, sounds, casings, projectiles, parameters, etc. So then I just need to create another class extending this gun class and fill my abstract methods up with what I need that particular gun to do.
So I made an abstract class to handle all of the internal code of the gun, if it shoots, if it needs bolt action and how long it has to wait, when to play the fire sound, etc. So basically the main gun class calls for the actual gun class (that is filled with abstract methods) for these parameters.
So my question is, if I am calling these abstract methods over and over again as needed throughout the code is it bad to have the following?
@Override
protected String getName() {
//This should be ok
return "Winchester";
}
@Override
protected boolean isBoltAction() {
//This should be ok
return false;
}
@Override
protected float getBoltActionTime() {
//This should be ok
return 0.5f;
}
@Override
protected Vector2 getPosOffset() {
//problem? or not?
return new Vector2(3,0);
}
If I call getPosOffset which I need it to be an Vector2 object for simplicity, the object itself shouldn't be too much expensive but would this create problems down the line? I ask because afaik even though the object itself doesn't change and is preset it is creating a new one everytime I call this method without really needing to, I could load a list and put everything there neatly without creating additional objects but that could make my abstract class kinda useless atleast for these methods. Is it bad to do this and should I change this? I've tried to see how many nanoseconds it takes if I load it up in a field vs this way and I saw no significant expense on time.
The game is running smoothly though but I am just not sure if my ocd is acting up or there is something problematic here.
UPDATE:
protected void update() {
//get position of player
pos.x = (float) (LevelManager.getPlayer().getPos().x);
pos.y = (float) (LevelManager.getPlayer().getPos().y);
//Tweener stuff to animate the recoil
posOffset.x = recoilTweener.tick();
//Fire time check
if (timePassed >= this.salvoDelay) {
//fire only if:
//if its automatic then allow it
//And
//if not needsBoltAction and we haven't "reloaded" the boltaction, these are Firearm fields not properties
//and
//if mouse left button is pressed
if (((this.automatic || !shooted) && (!needsBoltAction && !boltActionActive)) && Gdx.input.isButtonPressed(Buttons.LEFT)) {
//prevents hold firing for non automatic weapons
shooted = true;
//Tells us that we should fire
firing = true;
} else if (!Gdx.input.isButtonPressed(Buttons.LEFT)) {
//If the left mouse button isn't held lets reset
shooted = false;
}
} else {
//if time hasn't passed lets wait
timePassed += Math.min(Gameloop.getFrameTime(), this.salvoDelay);
}
//If we can fire and it was triggered above
if (firing) {
//Set time for salvo, in the sense each salvo shoots x projectiles within a set amount of time in between
timePassed += Math.min(Gameloop.getFrameTime(), this.salvoDelay + this.shotsPerSalvo * this.shotDelay);
//if we have shots to fire
//and
//if timepassed is bigger than the current shot * shotDelay
if (this.shotsPerSalvo > shotsFired && timePassed - this.salvoDelay >= shotsFired * this.shotDelay) {
if (this.shotDelay == 0) {
//Create projectile and set it up
createProjectile(this.shotsPerSalvo);
//Since we have 0 shotDelay we set this to false and reset the gun
firing = false;
shotsFired = 0;
timePassed = 0;
//"Ask" properties if we need boltAction
needsBoltAction = this.boltAction;
} else {
//Create each individual shot according to the time
createProjectile(1);
//Increase shotsFired for calculations
shotsFired++;
}
//If shotsFired reached the shotsPerSalvo, again each salvo has x shots to be fired in an instant (example shotguns, glocks in burst mode)
} else if (shotsFired >= this.shotsPerSalvo) {
//Reset weapon
firing = false;
shotsFired = 0;
timePassed = 0;
//"Ask" properties if we need boltAction
needsBoltAction = this.boltAction;
}
}
//BoltAction logic
//If needsBoltAction was triggered and we press R "reload" and wait (boltActionTime)
if (needsBoltAction && Gdx.input.isKeyJustPressed(Keys.R)) {
//Doing boltaction
boltActionActive = true;
//We dont need to press R again
needsBoltAction = false;
}
//We are doing the boltAction and not firing the gun
if (boltActionActive && !firing) {
//Add time
//Remember that timePassed was reset when all shots were fired on a salvo, in any situation
timePassed += Math.min(Gameloop.getFrameTime(), this.boltActionTime);
//If time was enough
if (timePassed >= this.boltActionTime) {
//We can shoot again since boltAction is done
boltActionActive = false;
timePassed = 0;
}
}
}
private void createProjectile(int amount) {
if (amount <= 0) {
Console.write(new Color(1,0,0,1),"Error: Projectile amount is invalid!");
}
if (getFireSoundFile() != null) {
getFireSoundFile().play();
}
if (this.casing != null) {
for (int i = 0; i < this.casingsPerSalvo; i++) {
ParticleManager.add(createCasing());
}
}
recoilTweener.reset();
for (int i = 0; i < amount; i++) {
Projectile p = createProjectile();
p.setup(LevelManager.getPlayer().getPos().x + (float) (Math.cos(Math.toRadians(WeaponManager.angle)) * this.shotStartingPosOffset.x + Math.sin(Math.toRadians(WeaponManager.angle)) * this.shotStartingPosOffset.y), LevelManager.getPlayer().getPos().y + (float) (Math.sin(Math.toRadians(WeaponManager.angle)) * this.shotStartingPosOffset.x + Math.cos(Math.toRadians(WeaponManager.angle)) * this.shotStartingPosOffset.y), WeaponManager.angle + Utils.getRandomFloat(this.spread,true));
WeaponManager.addProjectile(p);
}
}
private Casing createCasing() {
return new Casing(this.casing, this.pos);
}
private Projectile createProjectile() {
return new Projectile(this.projectile);
}
At the moment I have the properties read as so:
protected Firearm() {
loadParameters();
//other stuff
}
/**
* Loads all guns parameters from the abstract methods
* This is to only load these methods only once
*/
private void loadParameters() {
this.casing = getCasing();
this.magazineDrop = getMagazineDrop();
this.casingsPerSalvo = getCasingsPerSalvo();
this.unlimitedAmmo = hasUnlimitedAmmo();
this.projectile = getProjectile();
this.projectileDamage = getProjectileDamage();
this.spread = getSpread();
this.shotsPerSalvo = getShotsPerSalvo();
this.salvoDelay = getSalvoDelay();
this.shotDelay = getShotDelay();
this.automatic = isAutomatic();
this.fireSound = getFireSound();
this.reloadSound = getReloadSound();
this.name = getName();
this.boltAction = isBoltAction();
this.boltActionTime = getBoltActionTime();
this.shotStartingPosOffset = getShotStartingPosOffset();
this.recoilOffset = getRecoilOffset();
}
/**
* Gets bullet casing sprite
*/
protected abstract Casing getCasing();
protected Casing casing;
/**
* Gets magazine object
*/
protected abstract Magazine getMagazineDrop();
protected Magazine magazineDrop;
/**
* Number of casing drops per salvo
*/
protected abstract int getCasingsPerSalvo();
protected int casingsPerSalvo;
/**
* If the weapon has unlimited ammo
*/
protected abstract boolean hasUnlimitedAmmo();
protected boolean unlimitedAmmo;
/**
* Projectile texture path
*/
protected abstract Projectile getProjectile();
protected Projectile projectile;
/**
* Projectile Damage
*/
protected abstract float getProjectileDamage();
protected float projectileDamage;
/**
* Angle spread, angle added to each shot
*/
protected abstract float getSpread();
protected float spread;
/**
* Shots fired per salvo
*/
protected abstract int getShotsPerSalvo();
protected int shotsPerSalvo;
/**
* Not to be confused with getShotDelay
* This is the delay per salvo (each salvo will fire a number of shots)
*/
protected abstract float getSalvoDelay();
protected float salvoDelay;
/**
* Delay per shot on a salvo
*/
protected abstract float getShotDelay();
protected float shotDelay;
/**
* If true then the pistol is automatic, if false the pistol is semi-automatic
*/
protected abstract boolean isAutomatic();
protected boolean automatic;
/**
* If true then the pistol is automatic, if false the pistol is semi-automatic
* Note: this should only return the name of the file+extension, the file will be looked up in the internal folder "sounds/sfx/weapons/fire"
*/
protected abstract String getFireSound();
protected String fireSound;
/**
* If true then the pistol is automatic, if false the pistol is semi-automatic
* Note: this should only return the name of the file+extension, the file will be looked up in the internal folder "sounds/sfx/weapons/fire"
*/
protected abstract String getReloadSound();
protected String reloadSound;
/**
* Weapon's name
*/
protected abstract String getName();
protected String name;
/**
* If true then player will need to press R to reload
*/
protected abstract boolean isBoltAction();
protected boolean boltAction;
/**
* Time of bolt action
*/
protected abstract float getBoltActionTime();
protected float boltActionTime;
/**
* Firearm's bullet starting position offset
* Will automatically rotate angle
*/
protected abstract Vector2 getShotStartingPosOffset();
protected Vector2 shotStartingPosOffset;
/**
* Firearm's recoil in units to be subtracted to the weapons default offset position
* Will make the firearm go backwards and fowards with a tweener according to the units
* I am putting a vector2 just incase I need the firearm to recoil in the y vector but I atm dont see any use
*/
protected abstract Vector2 getRecoilOffset();
protected Vector2 recoilOffset;
Just incase you need the properties:
public class Winchester extends Firearm {
public Winchester() {
super();
}
@Override
public String getSprite() {
return "winchesterwh.png";
}
@Override
public String getSpriteDropped() {
return null;
}
@Override
protected Casing getCasing() {
return new Casing("sprites/weapons/casing/smallcasing.png", new Vector2(0,0));
}
@Override
protected Magazine getMagazineDrop() {
return null;
}
@Override
protected int getCasingsPerSalvo() {
return 1;
}
@Override
protected boolean hasUnlimitedAmmo() {
return false;
}
@Override
protected Projectile getProjectile() {
return new Projectile("sprites/weapons/projectiles/bullet.png", new Vector2(2,20), 50f, 0f, getProjectileDamage());
}
@Override
protected float getProjectileDamage() {
return 10f;
}
@Override
protected float getSpread() {
return 1f;
}
@Override
protected int getShotsPerSalvo() {
return 5;
}
@Override
protected float getSalvoDelay() {
return 0.25f;
}
@Override
protected float getShotDelay() {
return 0.25f;
}
@Override
protected boolean isAutomatic() {
return false;
}
@Override
protected String getFireSound() {
return "sniperShot.ogg";
}
@Override
protected String getReloadSound() {
return null;
}
@Override
protected String getName() {
return "Winchester";
}
@Override
protected boolean isBoltAction() {
return false;
}
@Override
protected float getBoltActionTime() {
return 0.5f;
}
@Override
protected Vector2 getPosOffset() {
return new Vector2(3,0);
}
@Override
protected Vector2 getShotStartingPosOffset() {
return new Vector2(0,5);
}
@Override
public float getDamage() {
return 0;
}
@Override
public Vector2 getRecoilOffset() {
return new Vector2(3,0);
} } // cant seem to get this bracket to behave
I think a solution for this were EVERYONE would be happy is if I used XML to load the properties instead
If I got your design correctly, you have a base class GunBase like this:
public abstract class GunBase
{
public abstract String getImageFileName();
}
and implementations of that like this:
public final class Winchester extends GunBase
{
@Override
public final String getImageFileName() { return "Winchester.png"; }
}
If GunBase really contains only those getter methods, it should not be a class at all, but an interface.
If GunBase really provides functionality that is only parameterised by the return values of those getters, those getters should not be overridden at all; instead the base class should get attributes for the respective values:
public abstract class GunBase
{
private final String m_ImageFileName;
protected GunBase( final String imageFileName )
{
m_ImageFileName = imageFileName;
}
public final String getImageFileName() { return m_ImageFileName; }
}
A derived class would then look like this:
public final class Winchester extends GunBase
{
public Winchester()
{
super( "Winchester.png" );
}
}
Confessed, the parameter list for the protected constructor can get inconveniently long in this case, but this can be solved by using a map with all the values instead of discreet arguments.
Now only those methods remain abstract that describe a behaviour that is different for each gun (really different, not only a different value for a parameter).
But if a gun is in fact nothing else than a collection of values, you should think about why you designed your guns as a class tree, and not as a collection of a class Gun where each instance describes one weapon type.