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

Adding Direct Vehicle Move waypoint #11

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Adding Direct Vehicle Move waypoint #11

wants to merge 11 commits into from

Conversation

CommandDDOS
Copy link

New Waypoint Vehicle Direct Move
Waypoint force vehicle drivers to move directly to waypoint position by setDriveOnPath
Waypoint create new groups for each driver in original group and assign them waypoint for better ability to control in zeus.

@CommandDDOS CommandDDOS added the feature New feature or request label Aug 14, 2024
@CommandDDOS CommandDDOS requested a review from a team as a code owner August 14, 2024 21:39
@CommandDDOS CommandDDOS requested review from 3Mydlo3 and removed request for a team August 14, 2024 21:39
@3Mydlo3 3Mydlo3 requested a review from veteran29 August 14, 2024 22:34
addons/waypoint/CfgWaypoints.cfg Outdated Show resolved Hide resolved
Comment on lines 4 to 5
class Move_vehicle {
displayName = CSTRING(Vehicle_Move_Name);
Copy link
Member

@veteran29 veteran29 Sep 12, 2024

Choose a reason for hiding this comment

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

Vehicle Move or Move Vehicle, stringtable naming is inconsistent with class. Also use camelpascal case for this (VehicleMove or MoveVehicle)

Copy link
Member

@3Mydlo3 3Mydlo3 Sep 12, 2024

Choose a reason for hiding this comment

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

Actually VehicleMove or MoveVehicle would be PascalCase, not camelCase.

addons/waypoint/XEH_postInit.sqf Outdated Show resolved Hide resolved
addons/waypoint/XEH_preInit.sqf Outdated Show resolved Hide resolved
* Return Value:
* none
*
*/
Copy link
Member

Choose a reason for hiding this comment

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

Comment does not follow the template, excess line between author and arguments.

Comment on lines 98 to 99
(driver _vehicle) disableAI "Path";
(driver _vehicle) enableAI "Path";
Copy link
Member

Choose a reason for hiding this comment

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

What does this achieve? Does this reset pathfinding?

Copy link
Author

Choose a reason for hiding this comment

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

Trying reset driver mostly in situationn where driver move vehicle super slowly after hitting object or moving super slowly in turning to waypoint

addons/waypoint/functions/fnc_moveVehicleWp.sqf Outdated Show resolved Hide resolved
addons/waypoint/functions/fnc_moveVehicleWp.sqf Outdated Show resolved Hide resolved
Comment on lines 71 to 72
_driver setVariable ["lambs_danger_disableAI", true];
(group _driver) setVariable ["lambs_danger_disablegroupAI", true];
Copy link
Member

Choose a reason for hiding this comment

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

You should save and restore values of these variables after the waypoint is completed.

addons/waypoint/functions/fnc_moveVehicleWp.sqf Outdated Show resolved Hide resolved
@@ -0,0 +1,15 @@
#define COMPONENT waypoint
Copy link
Member

@3Mydlo3 3Mydlo3 Sep 12, 2024

Choose a reason for hiding this comment

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

Personally I think plural would be more in-place for component concerning (possibly) multiple waypoints.

Suggested change
#define COMPONENT waypoint
#define COMPONENT waypoints

Keep in mind renaming folder.

Copy link
Member

Choose a reason for hiding this comment

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

I agree, do not forget to change pboprefix and folder names too.


if !(local _group) exitwith {false};

//vehicle list
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary comment, it's obvious what _vehicles = []; means.

Suggested change
//vehicle list

Comment on lines 1 to 4
## Waypoint

Waypoints:
- Vehicle Move - Order vehicle to move in strain line to target ignoring AI orders and situation. waypoint create pool of vehicles where group units are driver and create new group per vehicle
Copy link
Member

Choose a reason for hiding this comment

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

You should keep one empty line before and after any list in Markdown. Plus some spelling fixes.

I'm not sure what that does mean, so if my suggestion doesn't apply, please fix that.

Waypoint creates pool of vehicles where group units are driver and create new group per vehicle

Suggested change
## Waypoint
Waypoints:
- Vehicle Move - Order vehicle to move in strain line to target ignoring AI orders and situation. waypoint create pool of vehicles where group units are driver and create new group per vehicle
## Waypoints
New Waypoints:
- Vehicle Move - Orders vehicles to move in straight line to the waypoint position, ignoring AI orders and situation. Splits the original group to one group per vehicle for the duration of the move.

fast fixes from review

Co-authored-by: Filip Maciejewski <[email protected]>
Comment on lines 114 to 115
private _point1 = _vehicle getrelpos [2,0];
private _point2 = _vehicle getrelpos [3,0];
Copy link
Member

Choose a reason for hiding this comment

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

Please review your whole code for such cases, there's plenty of them. Please stick to camelCase/the same casing as on Biki.

Suggested change
private _point1 = _vehicle getrelpos [2,0];
private _point2 = _vehicle getrelpos [3,0];
private _point1 = _vehicle getRelPos [2,0];
private _point2 = _vehicle getRelPos [3,0];

[{
//condition
params ["_vehicle","_group"];
if !(waypointScript [_group,currentWaypoint _group] == QPATHTOF(functions\fnc_moveVehicleWP.sqf)) exitwith {true};
Copy link
Member

Choose a reason for hiding this comment

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

What if next waypoint is the same? Will there be 2 competing scripts? I don't see a valid reason for checking waypointScript. The only thing that should matter is active waypoint number (you could pass it as an argument to WUAE to have the old value for comparison).

Copy link
Author

@CommandDDOS CommandDDOS Sep 13, 2024

Choose a reason for hiding this comment

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

but waypoint gonna end only when WUAE is complete, so new wuae gonna be created for next waypoint
and this is to complete WUAE after ZEUS change Waypointype
edit. In arma 2.18 gonna be a option to replace it by passing _ThisScript to WUAE

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Nice, can be resolved then.

@3Mydlo3
Copy link
Member

3Mydlo3 commented Sep 13, 2024

A general comment from me would be to rethink how your waypoint function is composed.

  1. There's a separate logic for splitting group to one per vehicle before actual logic related to making vehicle move from point to point. Consider extracting one logic or the other to separate function.
  2. Your WUAE condition is enormous, checks many things and it's not clear what's the purpose of all that logic there. Could be moved to a separate function and cleaned up a bit. Possibly split into smaller parts as wait time handling seems to be very different part of the condition than first few ifs.
  3. If you're extracting functions (which I think would benefit readability of the code), consider renaming fnc_moveVehicleWp to fnc_waypointMoveVehicle to keep it in line with ZEN's scripted waypoints. As a bonus, all functions attached to waypoints will have waypoint prefix that will allow them to be distinguished from other utility functions (like your extracted conditions or splitting group per vehicle logic).

Comment on lines 125 to 126
_vehicle setvariable [QGVAR(ActiveVehicleMove),false];
},[_vehicle],0.2] call cba_fnc_waitAndExecute;
Copy link
Member

Choose a reason for hiding this comment

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

Please keep a space after ,. Also multiple occurrences.

Suggested change
_vehicle setvariable [QGVAR(ActiveVehicleMove),false];
},[_vehicle],0.2] call cba_fnc_waitAndExecute;
_vehicle setvariable [QGVAR(ActiveVehicleMove), false];
}, [_vehicle], 0.2] call cba_fnc_waitAndExecute;

Copy link
Member

Choose a reason for hiding this comment

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

I see veteran spared his time writing such comments and will have you fix all the issues that will be automatically reported by updated HEMTT. 🤣

@veteran29
Copy link
Member

@CommandDDOS please switch to HEMTT 1.3, I've merged master to your PR branch.

@veteran29 veteran29 self-requested a review September 13, 2024 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants