Skip to content

Conversation

@EttyKitty
Copy link
Owner

@EttyKitty EttyKitty commented Nov 4, 2024

  • Recentred all ship sprites and trimmed unused space.
  • Changed/fixed carrying display by moving it into a tooltip.
  • Changed/fixed the general ship list, to avoid text overlapping or going out of bounds.
  • I really wanted to format this file and refactor many things, but decided to keep this PR easy to review.

The code changes are simple, so you can just glance over them.
And the general look of the screen, I think it'll be easier to show before and after.

Before:

image

After:

image
image

@EttyKitty EttyKitty added PR: Enhancement Makes something better PR: Fix This is a fix for a bug labels Nov 4, 2024
@EttyKitty EttyKitty marked this pull request as draft November 4, 2024 03:06
@EttyKitty
Copy link
Owner Author

EttyKitty commented Nov 4, 2024

Edited the ship list a bit more:

Screenshot

image

@EttyKitty EttyKitty marked this pull request as ready for review November 4, 2024 04:36
@EttyKitty
Copy link
Owner Author

I'll pass the whole fleet tab file through a JS formmater when the PR is ready to merge.

@EttyKitty EttyKitty marked this pull request as draft November 4, 2024 04:39
@EttyKitty EttyKitty marked this pull request as ready for review November 4, 2024 05:04
@EttyKitty EttyKitty added this to the Next Patch milestone Nov 4, 2024
Copy link

@OH296 OH296 left a comment

Choose a reason for hiding this comment

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

cool stuff, got some suggestions very much in the take em or leave em category but worth considering imo

if (escort[k] == obj_controller.temp[40]) then instance_create(x, y, obj_temp7);
}
}
if (instance_exists(obj_temp7)) {
Copy link

Choose a reason for hiding this comment

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

if this is being redone can we please please deprecate the obj_temp7 usage

Copy link
Owner Author

@EttyKitty EttyKitty Nov 4, 2024

Choose a reason for hiding this comment

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

if this is being redone can we please please deprecate the obj_temp7 usage

I would be glad, but I have no idea what obj_temp7 is, or what it does, or how to deprecate it. I just copy-pasted the code chunk, so that its click detection is where the button coords are.

So if you know how to - you can do that and I cherry pick. Or create a separate PR, maybe?

Copy link

Choose a reason for hiding this comment

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

literally instead of using arrays to compile lists of stuff the guy made instances of these crappy obj_temp7 things also used to be obj_temp4,obj_temp5,obj_temp6 but i think i've removed most of these, i can maybe do this if it's an issue

if (point_and_click([_goto_button.x1, _goto_button.y1, _goto_button.x2, _goto_button.y2])) {
obj_controller.temp[40] = obj_ini.ship[i];
with(obj_p_fleet) {
for (var k = 0; k <= 40; k++) {
Copy link

Choose a reason for hiding this comment

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

without an array check length does this have a chance of causing crashes?

Copy link
Owner Author

Choose a reason for hiding this comment

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

without an array check length does this have a chance of causing crashes?

I'm not sure, but I haven't edited this code at all from where I copied it.
I assumed it being nested under 3 if checks and 1 for loop (that is limited as well) is enough of a failsafe.

@EttyKitty EttyKitty merged commit 7d10883 into compile/main Nov 4, 2024
@EttyKitty EttyKitty deleted the feat/fleet-tab branch November 4, 2024 22:43
@EttyKitty EttyKitty removed this from the Next Minor Version milestone Nov 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

PR: Enhancement Makes something better PR: Fix This is a fix for a bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants