Are Arroyo garden plant scripts obsolete?

[JuanCruzSanFran]

Arroyo garden has this nice feature that restocks broc flowers and xander roots every day. Apparently picking them up was supposed to be tracked by map variables MVAR_Current_Xander_Root and MVAR_Current_Broc_Flower through arroyo/aibroc.ssl¹ and arroyo/aixander.ssl, but those scripts are never linked (all new flowers and roots are created through create_object macro, so SID is always -1) and no scripts were linked in argarden.map either. And since those MVARs are never used elsewhere – that makes me think they were originally planned for checking if there's a need to spawn more, like geckos code here:
if (map_var(MVAR_Current_Gecko_Easy) < MIN_SMALL_GECKO) then begin
but eventually cut from game, cause right now they just spawn indefinitely each day on map enter.

1. aibroc.ssl has it's own bug – modifying wrong MVAR:

set_map_var(MVAR_Current_Xander_Root,map_var(MVAR_Current_Broc_Flower)-1);
set_map_var(MVAR_Current_Xander_Root,map_var(MVAR_Current_Broc_Flower)-1);
set_map_var(MVAR_Current_Xander_Root,map_var(MVAR_Current_Broc_Flower)+1);

while aixander.ssl supposed to work as intended

set_map_var(MVAR_Current_Xander_Root,map_var(MVAR_Current_Xander_Root)-1);
set_map_var(MVAR_Current_Xander_Root,map_var(MVAR_Current_Xander_Root)-1);
set_map_var(MVAR_Current_Xander_Root,map_var(MVAR_Current_Xander_Root)+1);

[burner1024]

Yes, looks like an oversight. Want to take a shot at this?

[JuanCruzSanFran]

Keep in mind that UP's scope is only to fix bugs. Not to add "nice to to have" things.

So what would be the best way to treat this issue: add originally intended functionality? If so – I can update the scripts, yet I have zero experience with map editing, so someone should link scripts to flower/root map objects (if any) in argarden.map.

[burner1024]

Yes, add the functionality. create_object_sid should be enough to attach the corresponging scripts. Need to verify it works as intended after.
Edit: just thought of something: I recall objects with scripts attached don't stack, so might need to detach them on pickup (could be the reason this wasn't completed in the first place).

[JuanCruzSanFran]

Yes, add the functionality. create_object_sid should be enough to attach the corresponging scripts. Need to verify it works as intended after.

I'll update the script, test it out and create a pull when ready.

Edit: just thought of something: I recall objects with scripts attached don't stack, so might need to detach them on pickup (could be the reason this wasn't completed in the first place).

That might be problematic, cause they both have drop procedure, which updates MVAR, but I guess we can cut that feature out. After all, when PC is dropping them – that doesn't mean he's is planting them back, right?

Btw, what version of sfall does UOP use: original or extended? Because I can't seem to find a way to remove script from object other than these two methods:

• remove_script
• creating another object on pickup and remove the original one

[burner1024]

That might be problematic, cause they both have drop procedure, which updates MVAR, but I guess we can cut that feature out. After all, when PC is dropping them – that doesn't mean he's is planting them back, right?

I agree.

[burner1024]

UPU uses original one, but they both have remove_script.

[JuanCruzSanFran]

Is it me, or remove_script is buggy when used inside pickup_p_proc? Makes you pickup item twice. I've written a workaround, found another script error and code is finally working, but it's kinda ugly. I would gladly refactor it later into a macro, but at least this version is usable.

There's just one thing bothering me – gecko population control code. Dumbed down:

if (current_geckos < MIN_GECKOS) then
   spawn random(MIN_GECKOS, MAX_GECKOS);
else if (current_geckos >= MIN_GECKOS and current_geckos < MAX_GECKOS)  then
   spawn random(0, MAX_GECKOS - MIN_GECKOS);
end

Resulting amount of geckos after spawn would always be from MIN_GECKOS to MAX_GECKOS+(MAX_GECOKS-MIN_GECKOS). And it would be a perfect candidate for refactoring, removing all this duplicated code with simple
spawn random(MIN_GECKOS - current_geckos, MAX_GECKOS + (MAX_GECKOS - MIN_GECKOS) - current_geckos)
IF it was an originally intended behaviour. But if original intention was "make more when less and less when more", code must stay. What are your thoughts?

P.S. Should I add this population control code to plants spawning? Cause rn they're just
if current < min then spawn random(min, max)

[burner1024]

Is it me, or remove_script is buggy when used inside pickup_p_proc? Makes you pickup item twice. I've written a workaround, found another script error and code is finally working, but it's kinda ugly. I would gladly refactor it later into a macro, but at least this version is usable.

I don't know. If it is, it would probably be good to open an issue in sfall repo.

IF it was an originally intended behaviour. But if original intention was "make more when less and less when more", code must stay. What are your thoughts?

My rule of thumb is that if I'm not sure it's a bug, it's probably not. Code is written explicitly, so I'm not sure.
(If anything, I think a making sense version would be spawn random(0, (MAX_GECKOS - MIN_GECKOS - current_geckos) );

P.S. Should I add this population control code to plants spawning? Cause rn they're just
if current < min then spawn random(min, max)

I wouldn't bother for the reason above, and anyway this is very minor.

[NovaRain]

I'd put item adding code in pickup_p_proc like this:

procedure timed_event_p_proc begin
   if (fixed_param == DUMMY_PARAM) then begin
      destroy_object(self_obj);
   end
end

procedure pickup_p_proc begin
   if (cur_map_index == MAP_ARROYO_WILDERNESS) then begin
      script_overrides;
      move_to(self_obj, 0, 1);
      set_map_var(MVAR_Current_Broc_Flower, map_var(MVAR_Current_Broc_Flower) - 1);
      add_obj_to_inven(source_obj, create_object(PID_BROC_FLOWER, 0, 0));
      add_timer_event(self_obj, 10, DUMMY_PARAM);
   end
end

[JuanCruzSanFran]

I'd put item adding code in pickup_p_proc like this:

procedure timed_event_p_proc begin
   if (fixed_param == DUMMY_PARAM) then begin
      destroy_object(self_obj);
   end
end

procedure pickup_p_proc begin
   if (cur_map_index == MAP_ARROYO_WILDERNESS) then begin
      move_to(self_obj, 0, 1);
      set_map_var(MVAR_Current_Broc_Flower, map_var(MVAR_Current_Broc_Flower) - 1);
      add_obj_to_inven(source_obj, create_object(PID_BROC_FLOWER, 0, 0));
      add_timer_event(self_obj, 0, DUMMY_PARAM);
   end
end

Alas. I've tested and you could manage to open inventory on the same tick as pickup and before timed_event_proc is called, drop item and pick it up again, effectively duplicating it. So unless it's surrounded by if (obj_is_carrying_obj(dude_obj, self_obj)) then begin check, it does not matter much where it's located.

Sorry, misread your code. Yeah, that might work.

I'm just fairly new to fallout scripting, been doing this for a week so I'm not sure how it all works yet. Does it mean every _proc procedure can be fully overridden without script_overrides disabling default behaviour e.g. putting item in inventory nevertheless on pickup_p_proc?

[burner1024]

Thank you for the pull request, it's now merged.