Comments

Technical Improvements #1#755

Open

Equinoxtic wants to merge 11 commits intoCodenameCrew:main from

Equinoxtic:main

Open

Technical Improvements #1 #755
Equinoxtic wants to merge 11 commits intoCodenameCrew:main from
Equinoxtic:main

Conversation

@Equinoxtic

Copy link

@Equinoxtic Equinoxtic commented Aug 23, 2025 •

edited

Loading

Hallo, I really like and love this engine, I thought I would try contributing to it as a gesture <3 (Also kind of my first contribution to any open-source project)

First, I wanted to improve the countdown code, which will allow to set certain parameters whether it can be enabled, plays sounds, animations, and etc. It may open up to more customizability when it comes to songs, or just some additional options

// PlayState Code Change Example
var countdown:Countdown = new Countdown({
	event: f(EVENT_NAME, EventManager.get(CountdownEvent).recycle(
		index,
		1,
		sounds[index],
		sprites[index],
		0.6, true, null, null, null)),
	enabled: true,
	playSound: true,
	animationPreset: DEFAULT,
	duration: (Conductor.crochet / 1000),
	speed: 1.0
});

Second, I wanted to compress and lessen some lines on the backend side of the code. This includes iterating over an array of strings instead of repeatedly using Sys.println(...), and making good use of conditional compilation flags to get the executables for the AsyncUpdater class

// Instead of this
Sys.println("-- Help --");
Sys.println("-help | Show this help");
Sys.println("-nocolor | Disables colors in the terminal");
Sys.println("-nogpubitmap | Forces GPU only bitmaps off");
Sys.println("-nocwdfix | Turns off automatic working directory fix");
// ...and so on
// Do this
final STRINGS:Array<String> = [
	"--- Codename Engine Command Line Help ---",
	"",
	"-help | Show this help",
	"-nocolor | Disables colors in the terminal",
	"-nogpubitmap | Forces GPU only bitmaps off",
	"-nocwdfix | Turns off automatic working directory fix",
	...
];
for (s in STRINGS) {
	Sys.println(s);
}

// UpdateUtil.hx (2 new functions)
public static function getExecName():String
{
	// Since previously, both linux and mac don't have the .exe extension, we might as well just compress this to a '#else' clause.
	return #if windows "CodenameEngine.exe" #else "CodenameEngine" #end; // weird syntax??? :skull: haxe is just full of surprises
}
public static function getGitExecName():String
{
	// Only add the suffix of the platform we need to get.
	var target:String = #if windows "windows.exe" #end
						#if mac "mac" #end
						#if linux "linux" #end;
	return 'update-${target}';
}
// AsyncUpdater.hx
public static var executableName:String = UpdateUtil.getExecName();
public static var executableGitHubName:String = UpdateUtil.getGitExecName();

Equinoxtic added 8 commits

August 23, 2025 16:48

@Equinoxtic


 Improve the getting of update executables

d20e9e3

@Equinoxtic


 whitespace :v

cd78437

@Equinoxtic


 usin' an array for this one

24537d1

@Equinoxtic


 Improve countdown code and create Countdown class

ae4ad98

@Equinoxtic


 Update Countdown.hx

18a88a5

- Adjustable Speed
- Name change preset: 'PULSING' -> 'BEATING'
- lil' bit of code refactoring

@Equinoxtic


 dampen beating size for countdown and fix easing

0f7724e

@Equinoxtic


 Create documentation for Countdown.hx

5c899ee

@Equinoxtic


 finishing touches

7716ef2

@Equinoxtic Equinoxtic changed the title ~~(削除) Some technical improvements (削除ここまで)~~ (追記) Technical Improvements (追記ここまで)

Aug 23, 2025

@Equinoxtic Equinoxtic changed the title ~~(削除) Technical Improvements (削除ここまで)~~ (追記) Technical Improvements #1 (追記ここまで)

Aug 23, 2025

@r6915ee

Copy link

Contributor

r6915ee commented Aug 23, 2025

This pull request can only be tested using the GitHub CLI in the most generic case, at least on my end. I prefer regular Git to handle these operations instead to offer the pure branch name. If I were you, I'd also probably split this into separate pull requests and have these be separate branches to offer the most secure way of handling merging, but that might just be me being picky about my branching as I usually am.

Now, for the review.

@Equinoxtic

Copy link

Author

Equinoxtic commented Aug 23, 2025 •

edited

Loading

This pull request can only be tested using the GitHub CLI in the most generic case, at least on my end. I prefer regular Git to handle these operations instead to offer the pure branch name. If I were you, I'd also probably split this into separate pull requests and have these be separate branches to offer the most secure way of handling merging, but that might just be me being picky about my branching as I usually am.

Now, for the review.

I'll take that as an advice, it's kind of my first time submitting PRs and whatnot and I only know the basics of Git and stuff T-T

@r6915ee

Copy link

Contributor

r6915ee commented Aug 23, 2025

This pull request can only be tested using the GitHub CLI in the most generic case, at least on my end. I prefer regular Git to handle these operations instead to offer the pure branch name. If I were you, I'd also probably split this into separate pull requests and have these be separate branches to offer the most secure way of handling merging, but that might just be me being picky about my branching as I usually am.

Now, for the review.

A small update, but the review was partially delayed because I had found an issue with my own custom version management program that prevented Lime from running properly in non-Visual Studio Code environments. I've issued a fix to it on my end, and, at the moment, I'm able to compile properly without issues. I'll review it once it compiles cleanly, since I had removed the export folder for troubleshooting prior.

r6915ee

r6915ee reviewed

Aug 23, 2025

View reviewed changes

Copy link

Contributor

@r6915ee r6915ee left a comment

Choose a reason for hiding this comment

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

The help message appears to work as intended. However, I do feel that the way the countdown is set up appears to be somewhat odd.

source/funkin/game/PlayState.hx Outdated

event.spriteTween = tween;

event.cancelled = false;

var countdown:Countdown = new Countdown({

event: gameAndCharsEvent("onCountdown", EventManager.get(CountdownEvent).recycle(

Copy link

Contributor

@r6915ee r6915ee Aug 23, 2025

Choose a reason for hiding this comment

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

I do wish to address this particular hunk. What would be the use case of not being able to manage most of the properties of Countdown in the event itself, like with the other events in PlayState?

Copy link

Author

@Equinoxtic Equinoxtic Aug 24, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

Hm, yeah, now that I think about it, it does feel like it's set up a little oddly. I thought of making an event as a parameter and just handling it (the event) within the Countdown class itself. Now that this has been brought up I start to wonder whether or not it'll break some backend functionality but it seemed to work as intended.

Reverting back to old CNE code where it has an event variable (in PlayState's countdown() method) and using that in the event parameter when making a new Countdown makes no difference (readability wise, I think it's better).

It's an odd setup I know 😅, and my apologies if I replied late. I was asleep by that time. I'll try coming up with an alternative solution later. For now, this is all I've really got.

r6915ee

r6915ee approved these changes

Aug 24, 2025

View reviewed changes

Equinoxtic added 2 commits

August 24, 2025 12:05

@Equinoxtic


 Merge remote-tracking branch 'upstream/main'

b803496

@Equinoxtic


 Revert back to CNE's implementation + compat with new Countdown class

892e3ac

@Raltyro Raltyro requested review from FuroYT and NexIsDumb

October 4, 2025 04:00

FuroYT

FuroYT reviewed

Oct 5, 2025

View reviewed changes

Copy link

Member

@FuroYT FuroYT left a comment

Choose a reason for hiding this comment

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

This is pretty good

source/funkin/game/Countdown.hx Outdated

Comment on lines 83 to 87

this.__createSprite();

}

@:noPrivateAccess

private function __createSprite():Void {

Copy link

Member

@FuroYT FuroYT Oct 5, 2025 •

edited

Loading

Choose a reason for hiding this comment

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

Why is this a function? Also why is it private within private access which hscript will bypass

Copy link

Author

@Equinoxtic Equinoxtic Oct 9, 2025

Choose a reason for hiding this comment

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

Hello! Sorry if it took a while since I didn't think this'd get really accepted as a pr, hehe.

I've been coding Haxe for a bit now, and I had this unnecessary habit of putting @:noPrivateAccess with private functions. I'll remove the annotation real quick.

source/funkin/backend/system/updating/UpdateUtil.hx Outdated

Comment on lines 34 to 47

public static function getExecName():String

{

// Since previously, both linux and mac don't have the .exe extension, we might as well just compress this to a '#else' clause.

return #if windows "CodenameEngine.exe" #else "CodenameEngine" #end; // weird syntax??? :skull: haxe is just full of surprises

}

public static function getGitExecName():String

{

// Only add the suffix of the platform we need to get.

var target:String = #if windows "windows.exe" #end

#if mac "mac" #end

#if linux "linux" #end;

return 'update-${target}';

}

Copy link

Member

@FuroYT FuroYT Oct 5, 2025

Choose a reason for hiding this comment

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

I find these comments unnecessary

@Equinoxtic


 Cleanup

ed46121

Labels

None yet

3 participants

@Equinoxtic @r6915ee @FuroYT

Uh oh!

Comments

Conversation

@Equinoxtic Equinoxtic commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r6915ee commented Aug 23, 2025

Uh oh!

Equinoxtic commented Aug 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

r6915ee commented Aug 23, 2025

Uh oh!

@r6915ee r6915ee left a comment

Choose a reason for hiding this comment

Uh oh!

@r6915ee r6915ee Aug 23, 2025

Choose a reason for hiding this comment

Uh oh!

@Equinoxtic Equinoxtic Aug 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@FuroYT FuroYT left a comment

Choose a reason for hiding this comment

Uh oh!

@FuroYT FuroYT Oct 5, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

@Equinoxtic Equinoxtic Oct 9, 2025

Choose a reason for hiding this comment

Uh oh!

@FuroYT FuroYT Oct 5, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

@Equinoxtic Equinoxtic commented Aug 23, 2025 •

edited

Loading

Equinoxtic commented Aug 23, 2025 •

edited

Loading

@Equinoxtic Equinoxtic Aug 24, 2025 •

edited

Loading

@FuroYT FuroYT Oct 5, 2025 •

edited

Loading