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

Space Update #1819

Merged
merged 72 commits into from
Jun 1, 2024
Merged

Space Update #1819

merged 72 commits into from
Jun 1, 2024

Conversation

Valo56
Copy link
Contributor

@Valo56 Valo56 commented Feb 2, 2024

For now all I've done is added chat commands for DM's to see ship stats and repair targeted ships.

More to come in time.

Valo56 and others added 9 commits February 2, 2024 01:03
…r PC's and NPC's. Added a skill level check to the shipstats command.
…service for config modules. Also updated combat lasers and ion cannons. Player ship stats have been updated in line with planned NPC and weapon changes.
…overhauled weapons, added a new feat, and added a new module category - configuration modules, to provide base stats to ships.
…riptions. Damage increases across the board as during testing fights were taking too long.
.RequiresTarget()
.Action((user, target, location, args) =>
{
if (GetIsDM(target) != true && GetIsDMPossessed(target) != true)
Copy link
Owner

Choose a reason for hiding this comment

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

Small stylistic thing, but would prefer this to be either:

!GetIsDM(target) && !GetIsDMPossessed(target)

or

GetIsDM(target) == false && GetIsDMPossessed(target) == false

The first one is more preferred over the second, but != true is weird :D

col.AddRow(row =>
{
row.AddButtonImage()
.BindImageResref(model => model.Configuration1Resref)
Copy link
Owner

Choose a reason for hiding this comment

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

Let's fix the tab formatting on this stuff. One tab for each dot method, new line in between elements.

_builder.Create("SPACE_SYBIL")
.AddItem("elec_ruined", 20)
.AddItem("ref_tilarium", 3);
_builder.Create("SPACE_BASIC_T1")
Copy link
Owner

Choose a reason for hiding this comment

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

Please review these loot tables for anything that needs to be marked as a rare item. Right now none of them are, so Treasure Hunter won't work.

@@ -1,9 +1,11 @@
using System.Collections.Generic;
using SWLOR.Game.Server.Core.NWScript.Enum;
using SWLOR.Game.Server.Entity;
using SWLOR.Game.Server.Enumeration;
Copy link
Owner

Choose a reason for hiding this comment

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

These changes aren't necessary. Please remove.


// Mirrored Plating 1
_builder.Create(RecipeType.ThermalArmor1, SkillType.Engineering)
.Category(RecipeCategoryType.ShipModule)
Copy link
Owner

Choose a reason for hiding this comment

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

Bunch of weird formatting with the tabs here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I've fixed the tabs now, please let me know if not, but none of this weirdness was showing for me in VS in the first place.

@@ -494,6 +602,60 @@ private void Tier3()
.Component("ref_idailia", 5)
.Component("elec_good", 3);

// Hypermatter Injector III
Copy link
Owner

Choose a reason for hiding this comment

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

I need these recipes added to the Bible too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll get a full list of updated stats and recipes your way soon. If you'd like it sooner than later I can get on that otherwise I was planning on waiting until wider testing was underway and further changes are unlikely.

_builder.Create("NPC_Sybil")
.ItemResref("npc_sybil")
.Name("NPC - Sybil")
.Appearance(AppearanceType.SWLORShipNeutralDropship)
Copy link
Owner

Choose a reason for hiding this comment

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

The appearance on all of these ships is missing. Can you double check on this? Are they actually showing the right models?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The right models display, the appearance in the code seems to be unnecessary for npc's, it's just pc's that need it.

@@ -55,10 +55,16 @@ private void MiningLaser(string itemTag, string name, string shortName, int requ
return "This mining laser is not powerful enough to harvest that asteroid.";
}

if (GetLocalBool(target, "BEING_MINED") == true)
Copy link
Owner

Choose a reason for hiding this comment

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

Drop the == true, not needed.

Please put "BEING_MINED" into a constant.

@@ -132,6 +138,7 @@ private void MiningLaser(string itemTag, string name, string shortName, int requ
var xp = Skill.GetDeltaXP(delta);

Skill.GiveSkillXP(activator, SkillType.Piloting, xp);
SetLocalBool(target, "BEING_MINED", false);
Copy link
Owner

Choose a reason for hiding this comment

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

I'd put this variable outside the PC check. If, somehow, a non-PC uses a mining laser the rock will be locked forever.

.Recast(12f)
.ValidationAction((activator, activatorShipStatus, target, targetShipStatus, moduleBonus) =>
{
var item = GetItemPossessedBy(activator, "ship_missile");
Copy link
Owner

Choose a reason for hiding this comment

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

Another situation where I'd set "ship_missile" to a constant so you are limiting the use of magic strings.

.EquippedAction((creature, shipStatus, moduleBonus) =>
{
shipStatus.ThermalDefense += armorBoost + moduleBonus;
shipStatus.EMDefense+= armorBoost + moduleBonus;
Copy link
Owner

Choose a reason for hiding this comment

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

Really minor but can you put a space before the plus sign here?

.ShortName(name)
.Texture(texture)
.Description($"Provides several base stats to a ship to allow it to fulfill a role and operate at full capacity: \n" +
$"Armor: +{armor} + Module Bonus * 4 \n" +
Copy link
Owner

Choose a reason for hiding this comment

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

Since this is being displayed to players, I'd recommend using "x4" instead of "* 4" simply because non-programmers might not understand what that notation means.

/// Sets the number of ship configuration nodes on this ship.
/// By default ships should have 1 configuration node.
/// </summary>
/// <param name="shipConfigurationNodes">The number of low power nodes to set.</param>
Copy link
Owner

Choose a reason for hiding this comment

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

Description for this param is wrong

var defense = Stat.GetDefense(target, CombatDamageType.Thermal, AbilityType.Vitality, defenseBonus);
if (attackRoll < 7)
{
attackType = "laser";
Copy link
Owner

Choose a reason for hiding this comment

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

Strings are very error prone and difficult to maintain. Please set up an enumeration - you can make it private within this class. I.E:

private enum AttackType
{
Invalid = 0,
Laser = 1,
...etc
}

var sound = EffectVisualEffect(VisualEffect.Vfx_Ship_Blast);
var missile = EffectVisualEffect(VisualEffect.Mirv_StarWars_Bolt2);

if (attackType == "laser")
Copy link
Owner

Choose a reason for hiding this comment

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

This method is getting very long. I would recommend splitting it up a bit. Perhaps a method for laser attacks, another for ion attacks, etc. There needs to be some organization here because it's going to be hard to maintain.

ApplyEffectToObject(DurationType.Instant, sound, target);
ApplyEffectToObject(DurationType.Instant, missile, target);

DelayCommand(0.3f, () =>
Copy link
Owner

Choose a reason for hiding this comment

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

What's the purpose of the delay?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The delay is the same as for combat lasers I believe (on phone atm hard to double check). Just has the damage and other effects occur after allowing time for the vfx to conclude.

Valo56 and others added 22 commits May 6, 2024 06:25
…eness of industrial modules for mining, reduced effectiveness of modulebonuses on lasers for mining.
…le as Dathomir, which makes sense to me based on what the Red Hand are). Music for remaining space areas adjusted.
…capital ship only, immobilizing and leaving vulnerable the mining vessel using it. Also adjusted Proton Bomb range based on feedback.
…es of bomber configurations. Fixed ship armor recipes. Increased missile drop rates. Reduced Intuitive Piloting to require 0 Piloting in response to feedback. Balance adjustments mostly involved buffs to turbolaser damage and increase to hull/SP extending modules and repairers.
…their target is dead. Balance update to corvettes, granting them additional cap.
.CanTargetSelf()
.ActivatedAction((activator, activatorShipStatus, target, targetShipStatus, moduleBonus) =>
{
var industrialBonus = Space.GetShipStatus(activator).Industrial;
Copy link
Owner

Choose a reason for hiding this comment

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

This has the potential to cause a null reference exception. Space.GetShipStatus can return a null in certain cases. I believe you need to add a null check here (and anywhere else) to bail out early if you don't have a valid ShipStatus object.

Copy link
Owner

Choose a reason for hiding this comment

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

Actually, looking at it closer you shouldn't need to call this at all. You already have activatorShipStatus available to you. Just use that and you don't need the null check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adjusted this.

{
_builder.Create(itemTag)
.Name(name)
.ShortName(name)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be shortName

{
_builder.Create(itemTag)
.Name(name)
.ShortName(name)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be shortName

{
_builder.Create(itemTag)
.Name(name)
.ShortName(name)
Copy link
Owner

Choose a reason for hiding this comment

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

This should be shortName

@zunath zunath merged commit b2eeb74 into zunath:master Jun 1, 2024
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.

None yet

2 participants