Skip to content

Created and implemented TrailType enum#3973

Closed
link5669 wants to merge 16 commits intoHarbourMasters:developfrom
link5669:trailType
Closed

Created and implemented TrailType enum#3973
link5669 wants to merge 16 commits intoHarbourMasters:developfrom
link5669:trailType

Conversation

@link5669
Copy link
Contributor

@link5669 link5669 commented Feb 23, 2024

This PR is in response to issue #1537. I've added and implemented a TrailType enum for weapon trails.

Build Artifacts

Copy link
Member

@inspectredc inspectredc left a comment

Choose a reason for hiding this comment

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

Looks good! Nice to have these magic numbers going away :D
Just left a few thoughts I had when going over this

void EffectBlure_Destroy(void* thisx) {
}

void updateTrailColor(const char* trailName, Color_RGBA8* color, int* changed, int* reset) {
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking these functions might be better moved to a c++ file. They're pretty nice to have and dont need to be specific to trails (I seem to recall seeing these patterns in other sections of the code). Small nitpick for this too would be the general naming format for functions in the codebase is generally pascal case, sometimes with an underscore as a separator for the object like family name and function purpose

Copy link
Contributor Author

@link5669 link5669 Feb 27, 2024

Choose a reason for hiding this comment

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

I looked for these patterns in the rest of the code and saw things like this:

Color_RGB8 cButtonsColor = {255, 160, 0}; 
if (CVarGetInteger("gCosmetics.Hud_CButtons.Changed", 0)) { 
   cButtonsColor = CVarGetColor24("gCosmetics.Hud_CButtons.Value", cButtonsColor); 
} 

Is it worth supporting RGBA and RGB in one function?

@Pepper0ni
Copy link
Contributor

Are you still interested in getting this ready to merge or should this PR and #3971 be open for replacement by a different user?

@aMannus aMannus added this to the 9.0.0 milestone Jan 11, 2025
@aMannus
Copy link
Contributor

aMannus commented Jan 17, 2025

Going to close this for now. If the creator (or anyone else) feels like picking this up again, feel free to open another PR.

@aMannus aMannus closed this Jan 17, 2025
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.

5 participants