-
Notifications
You must be signed in to change notification settings - Fork 737
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
(Ready) Adds more laser functionality to aircraft #7509
Conversation
if (((getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> "laser")) == 0) && | ||
{(getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> QGVAR(canSelect))) == 0}) exitWith {false}; | ||
if (((getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> "laser")) == 0) && | ||
(!(_currentShooter getVariable [QGVAR(hasLaserSpotTracker), false]) ) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(!(_currentShooter getVariable [QGVAR(hasLaserSpotTracker), false]) ) && | |
{!(_currentShooter getVariable [QGVAR(hasLaserSpotTracker), false])} && |
private _string = ""; | ||
if (_currentShooter getVariable [QGVAR(hasLaserSpotTracker), false]) then { | ||
private _LSTmessage = ""; | ||
if(_currentShooter getVariable [QGVAR(laserSpotTrackerOn), false]) then {_LSTmessage = localize LSTRING(LSTOn)} else {_LSTmessage = localize LSTRING(LSTOff)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Split into multiple lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be better than spliting it
if(_currentShooter getVariable [QGVAR(laserSpotTrackerOn), false]) then {_LSTmessage = localize LSTRING(LSTOn)} else {_LSTmessage = localize LSTRING(LSTOff)}; | |
private _LSTmessage = localize ([LSTRING(LSTOff), LSTRING(LSTOn)] select (_currentShooter getVariable [QGVAR(laserSpotTrackerOn), false])); |
params ["_args", "_pfID"]; | ||
_args params ["_vehicle"]; | ||
|
||
if (!(_vehicle getVariable [QGVAR(laserSpotTrackerOn), false])) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (!(_vehicle getVariable [QGVAR(laserSpotTrackerOn), false])) exitWith { | |
if !(_vehicle getVariable [QGVAR(laserSpotTrackerOn), false]) exitWith { |
addons/laser/stringtable.xml
Outdated
@@ -63,5 +63,11 @@ | |||
<Chinesesimp>雷射 - 循环切换雷射码 下</Chinesesimp> | |||
<Chinese>雷射 - 循環切換雷射碼 下</Chinese> | |||
</Key> | |||
<Key ID="STR_ACE_Laser_LSTOn"> | |||
<English>Laser Spot Tracker- On</English> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Space before dash
addons/laser/stringtable.xml
Outdated
<English>Laser Spot Tracker- On</English> | ||
</Key> | ||
<Key ID="STR_ACE_Laser_LSTOff"> | ||
<English>Laser Spot Tracker- Off</English> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
Space before dash
addons/laser/XEH_postInit.sqf
Outdated
["ace_laserOn", { | ||
params ["_uuid", "_args"]; | ||
private _vehicle = _args select 0; | ||
if(hasPilotCamera _vehicle) then { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(hasPilotCamera _vehicle) then { | |
if (hasPilotCamera _vehicle) then { |
addons/laser/XEH_postInit.sqf
Outdated
|
||
["Plane", "init", { | ||
params ["_vehicle"]; | ||
_hasPilotCamera = hasPilotCamera _vehicle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_hasPilotCamera = hasPilotCamera _vehicle; | |
private _hasPilotCamera = hasPilotCamera _vehicle; |
addons/laser/XEH_postInit.sqf
Outdated
|
||
["Air", "init", { | ||
params ["_vehicle"]; | ||
_hasPilotCamera = hasPilotCamera _vehicle; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_hasPilotCamera = hasPilotCamera _vehicle; | |
private _hasPilotCamera = hasPilotCamera _vehicle; |
@@ -40,8 +40,9 @@ if (isNull (ACE_controlledUAV param [0, objNull])) then { | |||
}; | |||
|
|||
TRACE_2("",_currentShooter,_currentWeapon); | |||
if (((getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> "laser")) == 0) && | |||
{(getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> QGVAR(canSelect))) == 0}) exitWith {false}; | |||
if (((getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> "laser")) == 0) && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (((getNumber (configFile >> "CfgWeapons" >> _currentWeapon >> "laser")) == 0) && | |
private _currentWeaponCfg = configFile >> "CfgWeapons" >> _currentWeapon; | |
if ((getNumber (_currentWeaponCfg >> "laser") == 0) && |
Save some repeated lookups. Same in the lower conditions.
Not sure if worth, but IMO also better readability through shother lines.
|
||
if((getPilotCameraTarget _vehicle) select 0) then { | ||
private _distance = ((getPilotCameraTarget _vehicle) select 1) distance _foundTargetPos; | ||
if(_distance > 0.5) then { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(_distance > 0.5) then { | |
if (_distance > 0.5) then { |
params ["_vehicle"]; | ||
|
||
_hasMarker = _vehicle getVariable [QGVAR(hasMarkerLaser), false]; | ||
if (! _hasMarker) exitWith {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (! _hasMarker) exitWith {}; | |
if (!_hasMarker) exitWith {}; |
|
||
params ["_vehicle"]; | ||
|
||
_hasMarker = _vehicle getVariable [QGVAR(hasMarkerLaser), false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_hasMarker = _vehicle getVariable [QGVAR(hasMarkerLaser), false]; | |
private _hasMarker = _vehicle getVariable [QGVAR(hasMarkerLaser), false]; |
private _enabled = _vehicle getVariable [QGVAR(laserMarkerOn), false]; | ||
_vehicle setVariable [QGVAR(laserMarkerOn), !_enabled]; | ||
|
||
private _markerMessage = if(_vehicle getVariable [QGVAR(laserMarkerOn), false]) then {localize LSTRING(LaserMarkOn)} else {localize LSTRING(LaserMarkOff)}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private _markerMessage = if(_vehicle getVariable [QGVAR(laserMarkerOn), false]) then {localize LSTRING(LaserMarkOn)} else {localize LSTRING(LaserMarkOff)}; | |
private _markerMessage = localize ([LSTRING(LaserMarkOff), LSTRING(LaserMarkOn)] select (_vehicle getVariable [QGVAR(laserMarkerOn), false])); |
} forEach _lightpoints; | ||
_vehicle setVariable [QGVAR(markerlightpoints), []]; | ||
|
||
if ( !(_vehicle getVariable [QGVAR(laserMarkerOn), false]) || !(alive _vehicle) ) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( !(_vehicle getVariable [QGVAR(laserMarkerOn), false]) || !(alive _vehicle) ) exitWith { | |
if (!(_vehicle getVariable [QGVAR(laserMarkerOn), false]) || !(alive _vehicle)) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified: !a || !b
== !(a && b)
private _num = 20; | ||
for "_i" from 1 to _num do { | ||
private _toPos = (_laserSource vectorAdd (_vector vectorMultiply _distance)) vectorAdd [sin(_i*(360/_num))*0.075,cos(_i*(360/_num))*0.075,0]; | ||
[ASLToAGL (_laserSource), ASLToAGL (_toPos), [0.8,1,0.8,1]] remoteExec [QFUNC(drawLaserLine), 0, false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a bad idea running a global remoteExec 20 times every frame, can you not just store/transfer the needed vars over and run the loop to call drawLaserLine locally or am I completely off base or incorrect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The way to reduce the calls would be to just to stuff them all into an array of arrays and have the draw command parse the entire array out within the draw script itself.
It'll reduce the network calls, but I feel it'd make the draw function a bit clunkier and, ultimately, all the draw events would have to happen anyway. I can cut the number down to somewhere between 12-15, but considering it's literally only being use to call a lightweight script I'm unsure how much impact it would have.
Though I honestly don't know, so if there's a second or third opinion on this, I'm happy to hear it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems like a bad idea running a global remoteExec 20 times every frame
The messages are only transmitted once per frame.. so doing 20 per frame doesn't make much sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea was to try and keep the draw script as lightweight as possible before, now instead of calling the draw script times, it only calls it once, but with an array of elements to iterate through.
Which method is better? I don't know.
addons/laser/CfgVehicles.hpp
Outdated
class ACE_Equipment { | ||
class ACE_ToggleMarkerLaser { | ||
displayName = CSTRING(laserMarkToggle); | ||
condition = QUOTE('Laserdesignator' in weapons ACE_player); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably want something like getNumber (configFile >> "CfgWeapons" >> (typeOf binocular ACE_PLAYER) >> QGVAR(whateverconfigname)) == 1
so that mods can add functionality without a headache
private _toPos = (_laserSource vectorAdd (_vector vectorMultiply _distance)) vectorAdd [sin(_i*(360/_num))*0.005,cos(_i*(360/_num))*0.005,0]; | ||
_list pushback [ASLToAGL (_laserSource), ASLToAGL (_toPos), [0.8,1,0.8,1]]; | ||
}; | ||
[_list] remoteExec [QFUNC(drawLaserLine), 0, false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't assume the fps on clients will be the same
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would FPS being different among clients have bearing on, or be consequence of, this code snippet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fnc_drawLaserLine does drawLine3D
this PFEH calls that on all machines
so if client A is running the PFEH with 20 fps, and client B has 50, B will have a flickering line
TBH I have no idea how this will work in real MP (with latency), there is little guarantee when the packets will arrive
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE also runs scheduled btw. REC is no quick fix either. RE is banned. The idea of remote execution on every frame is insanity.
addons/laser/XEH_postInit.sqf
Outdated
if (_ctrl) then {_key = _key + 486539264}; | ||
if (_shift) then {_key = _key + 704643072}; | ||
if (_alt) then {_key = _key + 939524096}; | ||
private _lightKeys = actionKeys "headlights"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does not work. Floats too big. actionKeys
command is unusable. This method cannot be saved. Rethink it from the beginning.
addons/laser/XEH_postInit.sqf
Outdated
|
||
["CAManBase", "init", { | ||
params ["_unit"]; | ||
findDisplay 46 displayAddEventHandler ["KeyDown", { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not savegame compatible.
private _toPos = (_laserSource vectorAdd (_vector vectorMultiply _distance)) vectorAdd [sin(_i*(360/_num))*0.05,cos(_i*(360/_num))*0.005,0]; | ||
_list pushback [ASLToAGL (_laserSource), ASLToAGL (_toPos), [0.8,1,0.8,1]]; | ||
}; | ||
[_list] remoteExec [QFUNC(drawLaserLine), 0, false]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RE is banned.
if (_weapons find "Laserdesignator_mounted" > -1) then { | ||
_weapon = "Laserdesignator_mounted"; | ||
} else { | ||
_weapon = "Laserdesignator_pilotCamera"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard coded classnames.
I don't like any of this. It is not ready to be reviewed. As it stands now, it needs to be completely rewritten. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought I was on armadev for a minute.
@commy2 |
It's poorly written. |
@@ -19,7 +19,7 @@ params [["_codeChange", 0, [0]]]; | |||
|
|||
TRACE_1("params",_codeChange); | |||
|
|||
if ((!alive ACE_player) || {!([ACE_player, vehicle ACE_player, []] call EFUNC(common,canInteractWith))}) exitWith {false}; | |||
if (( !alive ACE_player) || {!([ACE_player, vehicle ACE_player, []] call EFUNC(common,canInteractWith))}) exitWith {false}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stray space added?
private _string = ""; | ||
if (_currentShooter getVariable [QGVAR(hasLaserSpotTracker), false]) then { | ||
private _LSTmessage = localize ([LSTRING(LSTOff), LSTRING(LSTOn)] select (_currentShooter getVariable [QGVAR(laserSpotTrackerOn), false])); | ||
_string = format ["%1<br/>", _LSTmessage]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say format is probably more expensive than +
_string = format ["%1<br/>", _LSTmessage]; | |
_string = _LSTmessage + "<br/>"; |
GVAR(TrackerpfID) = [{ | ||
params ["_args", "_pfID"]; | ||
_args params ["_vehicle"]; | ||
if ( !(hasPilotCamera _vehicle)) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( !(hasPilotCamera _vehicle)) exitWith { | |
if (!(hasPilotCamera _vehicle)) exitWith { |
if ( !(hasPilotCamera _vehicle)) exitWith { | ||
[_pfID] call CBA_fnc_removePerFrameHandler; | ||
}; | ||
if (isNull(laserTarget _vehicle)) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (isNull(laserTarget _vehicle)) exitWith { | |
if (isNull (laserTarget _vehicle)) exitWith { |
if (isNull(laserTarget _vehicle)) exitWith { | ||
[_pfID] call CBA_fnc_removePerFrameHandler; | ||
}; | ||
if ( !((getPilotCameraTarget _vehicle) select 0)) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( !((getPilotCameraTarget _vehicle) select 0)) exitWith { | |
if (!((getPilotCameraTarget _vehicle) select 0)) exitWith { |
@@ -79,7 +84,9 @@ private _finalOwner = objNull; | |||
private _testPointVector = _posASL vectorFromTo _testPoint; | |||
private _testDotProduct = _dir vectorDotProduct _testPointVector; | |||
if ((_testDotProduct > _seekerCos) && {(_testPoint vectorDistanceSqr _posASL) < _seekerMaxDistSq}) then { | |||
_spots pushBack [_testPoint, _owner]; | |||
if(_owner != _ignoreBy) then { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if(_owner != _ignoreBy) then { | |
if (_owner != _ignoreBy) then { |
params ["_args", "_pfID"]; | ||
_args params ["_vehicle"]; | ||
|
||
if ( !(_vehicle getVariable [QGVAR(laserSpotTrackerOn), false]) || ( !alive _vehicle)) exitWith { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if ( !(_vehicle getVariable [QGVAR(laserSpotTrackerOn), false]) || ( !alive _vehicle)) exitWith { | |
if (!(_vehicle getVariable [QGVAR(laserSpotTrackerOn), false]) || (!alive _vehicle)) exitWith { |
if ((getPilotCameraTarget _vehicle) select 0) then { | ||
_angle = 0.75; | ||
}; | ||
private _pilotCameraLookPos = [sin(-deg(_pilotCameraRotation select 0)) * cos(-deg(_pilotCameraRotation select 1)), cos(-deg(_pilotCameraRotation select 0)) * cos(-deg(_pilotCameraRotation select 1)), sin(-deg(_pilotCameraRotation select 1))]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private _pilotCameraLookPos = [sin(-deg(_pilotCameraRotation select 0)) * cos(-deg(_pilotCameraRotation select 1)), cos(-deg(_pilotCameraRotation select 0)) * cos(-deg(_pilotCameraRotation select 1)), sin(-deg(_pilotCameraRotation select 1))]; | |
private _pilotCameraLookPos = [ | |
sin(-deg(_pilotCameraRotation select 0)) * cos(-deg(_pilotCameraRotation select 1)), | |
cos(-deg(_pilotCameraRotation select 0)) * cos(-deg(_pilotCameraRotation select 1)), | |
sin(-deg(_pilotCameraRotation select 1)) | |
]; |
addons/laser/initKeybinds.sqf
Outdated
@@ -13,4 +13,4 @@ | |||
[-1] call FUNC(keyLaserCodeChange); | |||
}, | |||
{false}, | |||
[18, [false, true, true]], false, 0] call CBA_fnc_addKeybind; // (ALT+CTRL+E) | |||
[18, [false, true, true]], false, 0] call CBA_fnc_addKeybind; // (ALT+CTRL+E) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing newline at EOF
@@ -63,5 +63,11 @@ | |||
<Chinesesimp>雷射 - 循环切换雷射码 下</Chinesesimp> | |||
<Chinese>雷射 - 循環切換雷射碼 下</Chinese> | |||
</Key> | |||
<Key ID="STR_ACE_Laser_LSTOn"> | |||
<English>Laser Spot Tracker: On</English> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm sure there is already a vanilla translation for On/Off that could be reused here, and get rid of the mostly duplicate entries here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would I find it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
either unpack and search the vanilla stringtables dta/languagecore.pbo or
https://synixebrett.github.io/stringtables/
STR_ACTION_LASER_OFF
is a allcaps OFF
STR_3DEN_Attributes_Radar_RadarOff_text
is a normal Off
Meh... Can't find a "On" though
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACE3/addons/map/stringtable.xml
Line 388 in 7cc168e
<Key ID="STR_ACE_Map_Action_NVGOn"> |
Here are On and Off, but I see the german translation for example wouldn't fit here as it doesn't mean "enabled/on" but more like "one" the number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACE3/addons/viewdistance/stringtable.xml
Line 255 in d8f18c6
<Key ID="STR_ACE_ViewDistance_object_off"> |
Same...
ACE3/addons/microdagr/stringtable.xml
Line 118 in 35e4c2a
<Key ID="STR_ACE_MicroDAGR_settingOff"> |
same... did we just copy the same On/Off translations into multiple components? 🤔
The title says ready, but there are still things that I already commented in my review 4 months ago, that are still wrong in multiple places (whitespace, codestyle) |
All the comments not having been made under the 48 hours previous to this one regarding code were each under comments marked 'outdated' or that had been removed, i.e. changes had been implemented on those files such that the code in question being questioned did not exist or (I had believed that) it had been rectified. |
When merged this pull request will:
[ ] Enable the usage of a marker laser (https://youtu.be/hv2gBHVXZds?t=169) for indication by plane to point to a location. (MP Compatible)[x] Adds marker laser to gunner stations (including helicopters and fixed wing turreted aircraft, i.e. Blackfish, 3rd party add-on second seat operators, e.g. F/A-18B/D).[ ] Adds marker laser option for handheld Laser designators (toggleable/keybind).Known issues:
Currently available on Pilot camera fixed wing craft only. (Will fix)Fixed.Some amount of marker laser/laser seeker detachment from target pod orientation when targeting on water. (Cannot fix?)Laser point tracking on certain buildings (e.g. Airstation Mike 26 Radome), where the geometry is transparent to the laser seeker does not generate a tracking point. (May attempt fix)Position of laser doesn't correspond to targeting pod location while at speed. (Cannot fix?)Fixed! Thanks @BaerMitUmlaut!Intended Behavior, which may be mistaken for bugs:
Marker laser doesn't shine when turned on, except when designating. This is primarily for code reasons. If there exists worry about laser designating the wrong target, switch your laser code.Marking this as 'Review Ready'.