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

Jetpack Thrust Orientation #7475

Merged
merged 9 commits into from
Mar 8, 2024

Conversation

mewacaser
Copy link
Contributor

Changes proposed in this pull request:

Changed the Regular jetpack mode to provide thrust towards the view's up vector instead of the world's.
Just a fun change really, I kept the speed to the same cap (actually lower as the total motion delta is clamped now and not just the vertical) so it's not giving super speed but you can maneuver by where you're facing now which is cool.

Also extracted the jetpack logic to an interface, still some more optimization that can be done though I think. For example ClientTickHandler is calling the getJetpack method (now getActiveJetpack) 3 times in its tick method. But it is working with the interface and it should allow jetpacks which don't use gas so a jetpack could be added which used the existing functionality and used a liquid, energy, or maybe even a cooldown.

Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

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

Overall definitely think changing to an interface makes things cleaner though I left a few comments. Unsure about the change to how the motion is handled though as I haven't gone ahead and tested it yet to see how it feels so after I do there is a chance I may end up having you revert it.

@mewacaser
Copy link
Contributor Author

I didn't really like how the refactor went honestly. I think there are still a lot of places it should be improved, but it's difficult in places like the client tick method where it searches the curios 3 times, because each time it is doing it in a different helper which makes sense as it is. I almost made this as a draft pr just because I figure there will be a decent number of changes still to go, but that's why it needs to be reviewed I guess...

As far as the actual thrust orientation change, it's fun and doesn't make it unbalanced. But it does make it different. My brother was thinking it should be a separate mode (maybe toggleable with a config?). I was also thinking it should be changed back to only clamp the y movement as the locomotion modules really get limited currently and other mods might also increase horizontal speed.

@mewacaser
Copy link
Contributor Author

I was also wondering if IJetpackItem (and/or IBlastingItem) should be put in the api in case someone wanted to implement it?

@omeranha
Copy link

hi @mewacaser I don't think this is the appropriate location, but, can I talk with you in private?

@mewacaser
Copy link
Contributor Author

hi @mewacaser I don't think this is the appropriate location, but, can I talk with you in private?

Sure

@omeranha
Copy link

hi @mewacaser I don't think this is the appropriate location, but, can I talk with you in private?

Sure

contact me in discord, please: oMeranha#8071

@mewacaser mewacaser force-pushed the feature/jetpack_orientation branch from b58e5b5 to 6b64c8a Compare May 27, 2022 16:30
@pupnewfster pupnewfster added this to the 10.2.2 milestone May 27, 2022
Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

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

Okay, so I finally got a chance to test this some and at least personally I don't feel like the changes to regular and then addition of ascend mode really fit that well in Mekanism (plus the movement in what is regular in the PR felt somewhat jerky and the like and was making me minorly nauseous. If you revert those changes though I will look into this PR again for purposes of it being more of a cleanup PR by use of creating the IJetpackItem interface.

@mewacaser
Copy link
Contributor Author

I agree that the refactor is entirely separate so I'll make a different pr for it. There's still a bunch to do on it though I think.

I'm still hoping you'll reconsider the thrust orientation though, everyone I had test it really liked it. So I'm gonna keep that branch around (but can close the pr if you want) on the off chance you come back around or decide it can be behind a config or something.

@mewacaser
Copy link
Contributor Author

I split the refactor into a separate pr.

Is there a good place I can get "pre-approval" on features before implementing them? I have more ideas, but some are iffy like this one on whether you would want them.

@pupnewfster
Copy link
Member

So I'm gonna keep that branch around (but can close the pr if you want) on the off chance you come back around or decide it can be behind a config or something.

I will think about this feature more for 1.19 but I highly doubt it will make it into 1.18

Is there a good place I can get "pre-approval" on features before implementing them? I have more ideas, but some are iffy like this one on whether you would want them.

Uh one of the easier spots to discuss things like that probably would be discord (especially in the testers channel as then it is easier for me to not miss the message if there is lots of chat going on)

@pupnewfster pupnewfster removed this from the 10.2.2 milestone May 29, 2022
@mewacaser mewacaser force-pushed the feature/jetpack_orientation branch from 6b64c8a to 128853f Compare June 7, 2022 22:20
@mewacaser mewacaser changed the base branch from 1.18.x to 1.20.x January 7, 2024 18:11
@mewacaser
Copy link
Contributor Author

I finally got around to updating and I started messing around with different thrust levels for the jetpack. This actually revealed a real defect that was really hard to notice without drastically increased thrust (at least double). That is just for elytra flight though and wasn't really noticeable at normal thrust.

I also changed out the y velocity cap for diminishing returns which I like much better. This was pretty much necessary for the higher thrust levels that the mekasuit can now get with the changes to the jetpack module stack size but I still think it is better even with the standard jetpack.

I'm still very much in favor of the vector mode. All of the people (4) who played with it on a server we ran on a homebrew jar seemed to prefer it over the regular mode for most things. I could see making it a config option like the atomic disassembler vein mining though if you still don't like it. Also I think it did get a bit smoother with the thrust changes but I might just be imagining that.

@pupnewfster
Copy link
Member

For some reason I didn't notice the notification when you updated it. Would you mind retargeting this against the 1.20.4 branch, and then I will try and look into it and do some testing myself?

@mewacaser mewacaser changed the base branch from 1.20.x to 1.20.4 February 28, 2024 04:59
@mewacaser
Copy link
Contributor Author

I just did a quick rebase without testing to make sure it builds. But it seems low risk that it would break so just let me know if I need to fix it.

@pupnewfster pupnewfster added 1.20 and removed 1.18 labels Feb 28, 2024
@pupnewfster pupnewfster added this to the 10.5.1 milestone Feb 28, 2024
@pupnewfster
Copy link
Member

I will try it more tomorrow, rather than now when I am tired, but from a very initial impression, some times vector feels nicer when it comes to going forwards, and sometimes it seems to send you in very random directions. For example if you look up enough then it makes you start going backwards?

Copy link
Member

@pupnewfster pupnewfster left a comment

Choose a reason for hiding this comment

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

On a bit more testing it seems that vector definitely feels nicer in some situations, though behaves weird in some. For example:

  • looking straight down
  • activating it (pressing space) while falling feels very disorienting in regards to which directions it causes you to go
  • Still feels a bit odd when if you look too high vertically it starts making you go backwards, but I think I sort of understand why it does

@pupnewfster pupnewfster modified the milestones: 10.5.1, 10.5.2, 10.5.3 Mar 1, 2024
@pupnewfster
Copy link
Member

Posting this here mainly so I remember to look into it, but the slider kind of is breaking down for the module tweaker

image

@pupnewfster
Copy link
Member

pupnewfster commented Mar 8, 2024

I uhh set the thrust to 8 for the jetpack on the mekasuit (on regular mode) pressed space very briefly and ended up at over y = 8,000. That doesn't seem correct. Testing again, tapping it takes me to like 3,000 and then momentum carries me up to about 8,000 without any input from me.

Pressing once on hover takes me to 1,600, and tapping sneak instantly brings me to the ground?

Edit: Ignore me, I just messed up an if statement for clamping usage when there isn't enough stored.

…hrust back to 0.15 so that the jetpack doesn't feel as slugish against gravity
@pupnewfster
Copy link
Member

I reverted the base thrust to 0.15 as the jetpack felt super slugish. This also made it so the vector mode didn't feel as off putting to me as I believe the issue was that it just didn't have enough thrust to reasonably be fighting gravity. Though I am a little unsure about the max thrust multiplier on the jetpack unit given it almost seems like it might be too much.

I also still need to do testing to see how all of this interacts with the elytra

@pupnewfster
Copy link
Member

I made the max thrust values a bit more linear and the maximum speed feels a bit more appropriate now. Main thing left I think is to figure out how to best handle the rendering of the modes in the module tweaker screen

@pupnewfster
Copy link
Member

image

@pupnewfster pupnewfster merged commit 00748b5 into mekanism:1.20.4 Mar 8, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants