-
Notifications
You must be signed in to change notification settings - Fork 16
Alpine 1.1 compat #342
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
base: master
Are you sure you want to change the base?
Alpine 1.1 compat #342
Conversation
…/fp_shotgun_reload.wav
rafalh
left a comment
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 PR is getting out of hands. It's really hard to review 1000+ changes (125 commits) multiple times. It takes me a lot of time and I'm still not sure if I didn't skip something. Can you split it into smaller PRs instead of making one big PR with everything? You add new stuff with every revision and fix unrelated stuff like adding const everywhere. If you must add it, okay, but not make huge PRs like this which changes 10 things at the same time. And all those commits have bad titles like "Update". Should I squash them and be left with one big commit in the git history? Every unrelated feature should have it's own commit/PR. Big PR for adding all Alpine stuff is a bad idea.
Small PR would also make it easier for me to find time to review it...
| const bool ir_scanner = (params.flags & MRF_SCANNER_1) != 0; | ||
| if (!ir_scanner) { | ||
| rf::Vector3 ambient_light{0.f, 0.f, 0.f}; | ||
| light_get_ambient(&ambient_light.x, &ambient_light.y, &ambient_light.z); |
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.
why it was moved? I can see in RF code that those builtin lights are used for characters too
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.
That is in render_v3d_vif which is for e.g. item pickups. Characters are lighted in render_character_vif.
| const bool ir_scanner = (params.flags & MRF_SCANNER_1) != 0; | ||
| if (!ir_scanner) { | ||
| const bool allow_full_bright = !(get_remote_server_info() | ||
| && !get_remote_server_info().value().allow_full_bright_entities); |
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.
there should be no negation or it should be "deny", not "allow", because currently it makes no 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.
const bool allow_full_bright = !(get_remote_server_info()
&& !get_remote_server_info().value().allow_full_bright_entities);
seems clearer in expressions e.g. if (config.full_bright && allow_full_bright) instead of if (config.full_bright && !no_full_bright)
const bool no_full_bright = get_remote_server_info()
&& !get_remote_server_info().value().allow_full_bright_entities;
| AlpineLevelInfoConfig g_af_level_info_config; | ||
|
|
||
| // trim leading and trailing whitespace | ||
| std::string trim(const std::string& str, bool remove_quotes = 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.
why this file recreates entire parsing module for no reason introducing bugs like not handling escape character in strings? You just copied it from Alpine right? Looks like AI slop :(
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 didn't have the energy to go through it since it was an isolated file.
| struct DashLevelProps | ||
| { | ||
| // backward compatible defaults | ||
| struct AlpineLevelProps { |
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.
of course Goober had to copy my way of handling that, but of course he didn't try to handle my chunk, right? I wonder why he didn't make new cyclic timers depend on rfl version
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.
Alpine Faction handles both chunks. legacy_cyclic_timers does depend upon RFL version. See event_type_forwards_messages_patch.
game_patch/misc/af_options.h
Outdated
| }; | ||
|
|
||
| struct AlpineLevelInfoConfig { | ||
| std::unordered_map<std::string, std::unordered_map<AlpineLevelInfoId, LevelInfoValue>> level_options{}; |
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.
what is this, some kind of cache that you never clean? another AI slop?
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.
Goober agreed it should be for the loaded level only. I've changed it.
game_patch/object/object.cpp
Outdated
| [](rf::Object* objp, const char* name, rf::VMeshType type) { | ||
| rf::VMesh* mesh = obj_create_mesh_hook.call_target(objp, name, type); | ||
| [] (rf::Object* const objp, const char* name, const rf::VMeshType type) { | ||
| const std::input_iterator auto level = g_af_level_info_config |
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.
level? that's a stupid name. Better mesh_replacements or replacements, because i'ts clearly not a level...
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.
Yeah I missed this somehow.
| int, | ||
| rf::Particle**, | ||
| rf::ParticleEmitter* | ||
| )> particle_create_level_emitter_hook{ |
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 name is confusing, it's pretty much a patch for ParticleEmitter::spawn method, but the name suggests it's some kind of constructor for particle emitter itself.
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.
It is called particle_create? What about particle_spawn_level_emitter_hook?
game_patch/rf/particle_emitter.h
Outdated
| float current_state_time; | ||
| bool active; | ||
| int field_144; | ||
| int active_distance;; |
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.
Double colon. Also is there a way to set this field in editor?
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.
Yes.
| && !get_remote_server_info().value().allow_full_bright_entities); | ||
| if (g_game_config.full_bright_entities && allow_full_bright) { | ||
| // For Direct3d 11, we set in `gr_d3d11_mesh.cpp`. | ||
| rf::Vector3& ambient_light = addr_as_ref<rf::Vector3>(0x01C3D548); |
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.
it's ambient_color, not to be confused with 005A38D4. I didn't check in PS2 code, but that's how it's named in my PC data
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.
From IDA:
ambient_color.x = g_ambient_light.x * 255.0;
ambient_color.y = g_ambient_light.y * 255.0;
ambient_color.z = g_ambient_light.z * 255.0;
Is it not the same?
| // RF does not allow triggers to activate events in multiplayer. | ||
| // In AF levels, we allow it, unless it is an event that would be | ||
| // problematic in multiplayer. | ||
| const rf::EventType event_type = static_cast<rf::EventType>(event->event_type); |
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.
(opinion) great idea, enable all events without any synchronization of the game state between server and client, especially late client. It probably introduces some quirks and even bugs, but who cares
Good point. But seems too late... |
Flags::allow_fb_meshFlags::allow_no_ssFlags::no_player_collideFlags::allow_no_mfFlags::click_limitaf_damage_notify_packetAlpineLevelProps::legacy_cyclic_timers$Lightmap Clamp Floor, and$Lightmap Clamp Ceiling,$Crater Texture PPM, and$Mesh Replacementfrom{map_name}_info.tblhit_sounds,weapon_screen_shake,full_bright_entities, andremote_server_flagscommandsI did not bother cleaning up
af_options.cppso it is just a copy paste.Resolves #182
Resolves #301