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

Add a flag that allows mods to create rotorlike aircraft #77663

Closed

Conversation

SariusSkelrets
Copy link
Contributor

@SariusSkelrets SariusSkelrets commented Nov 7, 2024

Summary

Features "Add a flag that allows mods to create rotorlike aircraft"

Purpose of change

I am planning to add a new vehicle part craftable by XE's inventors that would make a vehicle including said part flyable.

When designing how it would fly, it turned out that my intent for the behavior of that flight was incredibly similar to how helicopters fly.

Describe the solution

Add the ROTORLIKE_AIRCRAFT flag.
Installing a part with this flag allows the vehicle to fly in a similar way to helicopters.

By default, vehicles with this flag don't require any proficiency to fly but requirements can be defined in the vehicle parts.

They also can be repaired and modified with the only caveat of the removal of the fly-granting part removing the ability to fly until reinstalled.

Describe alternatives you've considered

Making this class a catch-all for all non-existing vehicles has been attempted. It was a horrendous idea.

Making a vehicle class that is obviously about a single vehicle part and nothing else was considered.

Testing

The game starts without throwing errors.
Regular helicopters still work as intended.
Vehicles with a part having this flag can fly.
Vehicles without rotors or this flag cannot attempt to lift.
Removed the part: Couldn't fly anymore.
Reinstalled the part: Could fly again.
Tried to fly the vehicle with a part having a defined proficiency without having said proficiency: couldn't fly it unless I had the proficiency.
Destroyed the part midflight: the aircraft crashed to the ground.
Destroyed one part midflight but not the other: the aircraft crashed to the ground even in case of a vehicle split.
Used autodrive: works without issue. It needed some more code for autodrive to handle a vehicle with both wheel and flight but it now uses the autodrive air speed as intended and only slows down when turning or completing its path.
Used autodrive while in a helicopter: worked without issue.
Used autodrive while in a car: worked without issue.

Additional context

@github-actions github-actions bot added <Documentation> Design documents, internal info, guides and help. [JSON] Changes (can be) made in JSON Vehicles Vehicles, parts, mechanics & interactions [C++] Changes (can be) made in C++. Previously named `Code` [Markdown] Markdown issues and PRs <Enhancement / Feature> New features, or enhancements on existing json-styled JSON lint passed, label assigned by github actions labels Nov 7, 2024
@GuardianDll
Copy link
Member

something went clearly wrong with your astyle

@SariusSkelrets
Copy link
Contributor Author

SariusSkelrets commented Nov 7, 2024

That's what I noticed. It appears that my current astyle isn't the right one, so I'll see if I can modify the astyle included included in my instance of visual studio and if I can't, then I'll have to revert and manually fix that push.

Edit: found it. My version of astyle was too old. Now it's fixed

@github-actions github-actions bot added Info / User Interface Game - player communication, menus, etc. Map / Mapgen Overmap, Mapgen, Map extras, Map display labels Nov 8, 2024
@kevingranade
Copy link
Member

I was under the impression that you'd be implementing some distinct form of flight, not just bypassing the checks added for helicopters.

I'm not interested in maintaining hacks in game to implement these bypasses.

@SariusSkelrets
Copy link
Contributor Author

I am implementing a distinct type of flying vehicles intended for mainlined mods. Non-rotor aicraft is anything that can fly but isn't using rotors to do so. Jury-rigged mad science aircraft? Yes. Sci-fi regular science aircraft? Yes. Any other thing that can fly? Yes.

This is distinct enough from helicopters to require separate code but similar enough to not need to fully duplicate the code where adding to the check can do the same. Aircraft and rotorcraft share the same main function and goal: lifting a valid vehicle from the ground so there's some overlap unlike, say, the systems for ground vehicles and for water vehicles.

By the way, I have been informed that the function of at least one of the helicopter checks is to forbid anything that isn't a flyworthy helicopter from flying and to forbid anything without a rotor from being considered a flying vehicle so any such checks need to be bypassed for non-rotor aircraft to be possible at all.

Flat-out removing said checks isn't an option either, as according to @RenechCDDA all vehicles are flyable by default. They just can't attempt to fly if they don't have rotors, meaning that I need to add something that can be used to allow a vehicle to fly while keeping the code that keeps regular vehicles from flying.

@kevingranade
Copy link
Member

What is the distinct type of flying vehicle then, because it looks like you're just doing "make it work like a helicopter but bypass the rotor checks"?

@SariusSkelrets
Copy link
Contributor Author

My goal is to add the required infrastructure to simulate any new vehicle type without every single new type needing their own code for that through a polyvalent flag.

Aircraft is intended to allow anything to be made flyable in a way that can be adapted to any modded vehicle that would use it. Rotorcraft is rotorcraft, but aircraft can be anything the modder wishes to make and allows it to be done in a 100% Json way.

In my case, I have plan involving XE's inventors and making aircraft. The need for aircraft has come up for Magiclysm and Aftershock too, as I read on the discord. The alternative is fully duplicating the entire flight code at least three times (jury-rigged sci-fi/regular sci-fi/magic) which ins't sustainable long-term.

The aircraft system handle anything that can be described as "this single-tile part allows flight, losing that part loses the ability to fly this vehicle" without needing to mostly copy-paste the same code over and over.

The main overlap with how helicopters work is that both allow a vehicle to fly if the conditions are met.

@kevingranade
Copy link
Member

The existing system is Not Fit For Purpose for representing arbitrary aircraft, that's why it has all these limitations.
If you want to represent a ligher-than-air baloon, that works completely differently from a helicopter and they will have big chunks of separate code. If you want a fixed-wing aircraft that is again TOTALLY different, if you want a magic carpet it's different again, a gyrocpter, different.

I'm not merging a PR that goes, "you can be anything you want as long as it flies kind of like a jank helicopter and now we're pretending that's ok".

@SariusSkelrets
Copy link
Contributor Author

if you want a magic carpet it's different again

This is what I'm aiming for: to allow the things that allow abstraction and are little more than "it flies because it does", the kind of vehicle that would never, ever be part of non-modded CDDA but has its place in mainlined mods while requiring little more than being able to quit the ground.

These are the vehicles this PR intend to allow. It's not a system for balloons, nor for fixed-wing aircrafts nor gyrocopters. It's a system for mod-makers wanting to add their own handwaved flying vehicle even if they're not based on anything that exists.

If I'm allowed to continue work on this PR, I'll add a very clear note about how this flag is meant to never be used for anything not within a mod along with bunch of renaiming so it's clearly mod-only.

I feel that it would address the valid point you're raising about the content of this PR being seen as nothing but a shortcut to add planes. dirigibles, etc to the main game without the proper systems of each.

@SariusSkelrets SariusSkelrets changed the title Add a flag for the creation and flight of non-rotor aircraft [WIP] Add a flag that allows mods to add non-rotor aircraft Nov 12, 2024
@emixa-d
Copy link

emixa-d commented Nov 25, 2024

I guess what kevingranade is thinking of, is that most aircraft doesn't move the same way a ground vehicle or helicopter does, so you can't simply abstract things, you need to actually implement the per-aircraft-type behaviour.

It's a system for mod-makers wanting to add their own handwaved flying vehicle even if they're not based on anything that exists.

And this is insufficient information - even if it's magic / handwavium, it still needs to be known how the vehicles moves (effect) even if cause of the movement is magic (especially if it's magic, because then it easily might behave differently from real-life things).

E .g., can the aircraft be moved in all directions (UFOs?) or does it need to rotate first to point in the right direction, does it spin (some UFOs), are there extrusions that can be damaged by nearby trees (cata rotors), are there height limitations (hovercraft?), how does energy consumption scale with velocity, acceleration, air resistance, distance to ground, is acceleration instantaneous, does it perform many small teleports or does it have well-defined speeds? If some of the engines are broken or turned off, does it only reduce thrust (cars) or does it reduce in what directions rotation and thrust can be applied (some spacecraft designs, also bad for airplanes I think)?

In any case, the C++ code needs to know these factors. Perhaps it could delegate it to JSON code, ...

Rotorcraft is rotorcraft, but aircraft can be anything the modder wishes to make and allows it to be done in a 100% Json way.

... but that delegation hasn't actually been implemented by this PR.

@SariusSkelrets
Copy link
Contributor Author

The PR is in the middle of being rewritten to better reflect its intent: allowing vehicles that have no real-world equivalent and only these, the vehicles that cannot be based on IRL flying vehicles, to be added to mods. The description is still the old one as I'm trying to get the compiled game to open again first before modyfing the PR in case the new description needs to be adjusted (said description already needs a heavy rewrite).

What I had in mind when implementing the abstracted_aircraft vehicle class are the myriad of vehicles that are incredibly similar in function to rotorless helicopters: sci-fi ships, magic brooms, UFOs, magic carpets, steampunk jetpack chairs, etc, which all work with the following mechanics:

  • They can ascend without horizontal movement
  • They can maintain a stationnary flight
  • They need to turn to face the direction they want to go forward (even UFOs rotate to face their destination)
  • Rotating the vehicle in flight is impacted by the vehicle's speed
  • Acceleration is quick but not instant and ground distance doesn't noticeably impact it
  • They are affected by air resistance as mundane objects would
  • Power/fuel efficiency is dictated by their engines
  • Their flight comes from one or multiple relatively small parts, placed inside or outside the vehicle
  • As long as said parts aren't compromised directly or indirectly the aircraft still flies
    -The parts can work at less than their full power
  • Destroying one of said parts causes the aircraft to crash to the ground (even rocket-propelled aircrafts quickly crash if one of the propellers is destroyed), so they will always be activated during flight
  • Etc.

This is why they're all grouped up in the same system: despite the lack of real-world examples, there's a huge pile of fiction examples and they all behave the same way flightwise.

The Json plans have also changed to limit scope creep. It turned out that allowing any non-rotor vehicle at all was a much bigger task than planned, so Jsonizing it will have to wait. The current goal is to get the system to work at all, as it currently doesn't.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 26, 2024
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Nov 30, 2024
@emixa-d
Copy link

emixa-d commented Dec 1, 2024

I would make rotorcraft (helicopter) be a special case of rotorlike_aircraft (helicopters behave like helicopters, after all). That would simply most checks (except the one at // If a part allows to fly, losing it removes the ability to fly). Now that the PR is a bit more limited in scope, I don't expect to have anything else to comment.

Copy link
Contributor

github-actions bot commented Dec 1, 2024

Spell checker encountered unrecognized words in the in-game text added in this pull request. See below for details.

Click to expand
  • Adding a part with this flag makes the vehicle a rotorlike aircraft (any flying vehicle that flies as if it was a helicopter without rotors), intended for modded content. Breaking or removing this part will cause the vehicle to no longer be able to fly.

This alert is automatically generated. You can simply disregard if this is inaccurate, or (optionally) you can also add the new words to tools/spell_checker/dictionary.txt so they will not trigger an alert next time.

Hints for adding a new word to the dictionary
  • If the word is normally in all lowercase, such as the noun word or the verb does, add it in its lower-case form; if the word is a proper noun, such as the surname George, add it in its initial-caps form; if the word is an acronym or has special letter case, such as the acronym CDDA or the unit mW, add it by preserving the case of all the letters. A word in the dictionary will also match its initial-caps form (if the word is in all lowercase) and all-uppercase form, so a word should be added to the dictionary in its normal letter case even if used in a different letter case in a sentence.
  • For a word to be added to the dictionary, it should either be a real, properly-spelled modern American English word, a foreign loan word (including romanized foreign names), or a foreign or made-up word that is used consistently and commonly enough in the game. Intentional misspelling (including eye dialect) of a word should not be added unless it has become a common terminology in the game, because while someone may have a legitimate use for it, another person may spell it that way accidentally.

@SariusSkelrets SariusSkelrets changed the title [WIP] Add a flag that allows mods to add non-rotor aircraft Add a flag that allows mods to create rotorlike aircraft Dec 1, 2024
@RenechCDDA
Copy link
Member

RenechCDDA commented Dec 1, 2024

I feel like this hasn't been asked, so I will ask: Why do you need an aircraft that behaves exactly like a helicopter but is not (internally) called a helicopter and copies 99% of the helicopter code? Why can't you just implement as a helicopter, without any new C++?

@SariusSkelrets
Copy link
Contributor Author

I would make rotorcraft (helicopter) be a special case of rotorlike_aircraft (helicopters behave like helicopters, after all). That would simply most checks (except the one at // If a part allows to fly, losing it removes the ability to fly). Now that the PR is a bit more limited in scope, I don't expect to have anything else to comment.

So instead of adding OR checks, I make the rotorlike_aircraft class include the rotorcraft class for most purposes and add checks looking for rotors in the code lines where having rotors or not matter?

I can see if I manage to do that. I have in mind placing a check to see if one of the two flags is present (for most behaviors), a check to determinate when regular rotors are used (so helicopters keep their function) and code to prevent both flags from being applied to the same vehicle (with an error if both are defined on the same part)

@emixa-d
Copy link

emixa-d commented Dec 1, 2024

So instead of adding OR checks, I make the rotorlike_aircraft class include the rotorcraft class for most purposes and add checks looking for rotors in the code lines where having rotors or not matter?

Yes, that's it. Although, keep in mind I'm not familiar with cata's vehicle code.

@RenechCDDA:

I feel like this hasn't been asked, so I will ask: Why do you need an aircraft that behaves exactly like a helicopter but is not (internally) called a helicopter and copies 99% of the helicopter code? Why can't you just implement as a helicopter, without any new C++?

It is actually asked: #77663 (comment) (and initially (emphasis on initially) without a proper answer).

It's the 1% that's important:

then another PR to add the parts proper, including the inventor's antigravity reactionless drive that will work exactly as I described.

This sounds like it doesn't have rotors, so the rotor collision checks don't make sense in this case (I haven't checked if they were removed for non-helicopters though).

Also, from the very first comment:

By default, vehicles with this flag don't require any proficiency to fly but requirements can be defined in the vehicle parts.

They also can be repaired and modified with the only caveat of the removal of the fly-granting part removing the ability to fly until reinstalled.

These properties aren't like helicopters either.

@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 1, 2024
@github-actions github-actions bot removed the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 1, 2024
@github-actions github-actions bot added the astyled astyled PR, label is assigned by github actions label Dec 2, 2024
@github-actions github-actions bot added the BasicBuildPassed This PR builds correctly, label assigned by github actions label Dec 2, 2024
@SariusSkelrets SariusSkelrets marked this pull request as ready for review December 3, 2024 14:09
Copy link
Member

@RenechCDDA RenechCDDA left a comment

Choose a reason for hiding this comment

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

I'm going to be blunt with you at this point: This seems to be above your current capabilities.

You clearly do not understand the code you are modifying, nor the concepts they represent (e.g. rotorcraft lift). I'm not saying that to denigrate you, or say that's not allowed. This is a very complicated part of the codebase and anything we add here only makes that worse.

You have to understand that your responsibility in adding code is not just to do the thing you want. It's also got to be maintainable. You have to make sensible assumptions, and others need to be able to follow your code... even if they're pretty ignorant of the what the code is actually doing/supposed to do.

So far, you have not left that impression on me. Even if you are able to address all the concerns I have raised, I'm not the only person you need to convince. I'm not even the most critical one!

Simply, I do not see any likely futures where you can get this PR into a shape that it would be merged.

My recommendation to you would be to shelve this project, and consider revisiting it when you have a better understanding of C++ and the codebase.

bool is_wiring = parts[split_part0].info().base_item == itype_wall_wiring;
bool is_appliance = parts[ split_part0 ].info().has_flag( flag_APPLIANCE );
bool is_wiring = parts[ split_part0 ].info().base_item == itype_wall_wiring;
bool is_rotorlike_aircraft = parts[ split_part0 ].info().has_flag( flag_ROTORLIKE_AIRCRAFT );
Copy link
Member

Choose a reason for hiding this comment

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

Appliances can use front() because they are always one single vehicle part. Therefore front() is always the one and only vehicle part in the parts array.

This is not true in your case.

@@ -1572,6 +1577,10 @@ int vehicle::install_part( const point_rel_ms &dp, vehicle_part &&vp )
debugmsg( "installing %s would make invalid vehicle: %s", vpi.id.str(), valid_mount.str() );
return -1;
}
if( vpi.has_flag( flag_ROTORLIKE_AIRCRAFT ) ) {
// Installing a part that grants flyworthiness allows the vehicle to fly.
set_flyable( true );
Copy link
Member

Choose a reason for hiding this comment

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

This should just be overridden in vehicle::is_flyable()

// so ground speed is used here.
int vehicle::max_rotorlike_velocity( const bool fueled ) const
{
const double max_air_mps = std::sqrt( max_ground_velocity( fueled ) / coeff_air_drag() );
Copy link
Member

Choose a reason for hiding this comment

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

Why are we checking our possible ground velocity? This is only called when we are is_flying.

// so ground speed is used here.
int vehicle::safe_rotorlike_velocity( const bool fueled ) const
{
const double max_air_mps = std::sqrt( safe_ground_velocity( fueled ) / coeff_air_drag() );
Copy link
Member

Choose a reason for hiding this comment

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

Ditto, why check the ground?

bool vehicle::is_rotorlike_aircraft() const
{
// checks if the aircraft is a helicopter or works like one
return ( ( !rotorlike_aircraft.empty() && has_driver() ) || ( !rotors.empty() && has_driver() &&
Copy link
Member

Choose a reason for hiding this comment

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

Clang error that would need to be resolved.

int vehicle::max_velocity( const bool fueled ) const
{
if( is_flying && is_rotorcraft() ) {
return max_rotor_velocity( fueled );
} else if( is_flying && !is_rotorcraft() ) {
Copy link
Member

Choose a reason for hiding this comment

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

!is_rotorcraft() doesn't make much sense here, it should be is_rotorlike_aircraft().

@@ -564,7 +565,9 @@ void vehicle::thrust( int thd, int z )
// helicopters improve efficiency the closer they get to 50-70 knots
// then it drops off as they go over that.
// see https://i.stack.imgur.com/0zIO7.jpg
if( is_rotorcraft() && is_flying_in_air() ) {
// rotorlike aircraft are intended to be mechanically similar to helicopters
// so they get the same efficiency factor
Copy link
Member

@RenechCDDA RenechCDDA Dec 9, 2024

Choose a reason for hiding this comment

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

.......why?

Again, this is a rotor physics issue. If you aren't using rotors, you should not be dealing with their physics.

Just to further clarify: As the rotor goes faster the relative speed of the airflow over its lifting surface goes down, which makes it produce less lift. Hence the "pushing power" at 200mph is not equivalent to the very same "pushing power" at 500mph.

You do not have a rotor. This should not apply to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
astyled astyled PR, label is assigned by github actions BasicBuildPassed This PR builds correctly, label assigned by github actions [C++] Changes (can be) made in C++. Previously named `Code` <Documentation> Design documents, internal info, guides and help. <Enhancement / Feature> New features, or enhancements on existing Info / User Interface Game - player communication, menus, etc. [JSON] Changes (can be) made in JSON json-styled JSON lint passed, label assigned by github actions Map / Mapgen Overmap, Mapgen, Map extras, Map display [Markdown] Markdown issues and PRs Vehicles Vehicles, parts, mechanics & interactions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants