Conversation
|
Important Review skippedToo many files! This PR contains 251 files, which is 101 over the limit of 150. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (49)
📒 Files selected for processing (251)
You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use luacheck to improve the quality of Lua code reviews.Add a configuration file to your project to customize how CodeRabbit runs luacheck. |
|
While reviewing the files on GitHub, I noticed several issues (e.g., incorrect spacing and unwanted symbols). Additionally, some values were modified since my last update that were not intended to be changed (e.g., HP). |
|
Very nice writeup on the changes, good job! |
Lowercase file paths Revert whitespace changes Revert changes to copyright symbol in unit scripts
lL1l1
left a comment
lL1l1
left a comment
There was a problem hiding this comment.
Aeon factories reviewed:
Aeon T1->T2 land factory upgrade detaches the left arm which looks weird because its physically impossible and the factory becomes asymmetrical during the upgrade. Not sure about the solution, it is a difficult problem.
Aeon T1 navy fac water effects and hitbox do not match the mesh.
Do you know if Scrolling = true was used on the aeon factor lod 0 meshes?
|
@IL1I1 thank you very much for your testing. Aeon T1 Land Factory Upgrade Unfortunately, while creating the animation, I couldn’t come up with a better solution than the current one. Previously, both roof elements kept their positions during the upgrade process. Since the tower positions change in the T2 HQ and Support models, I considered this approach to be the most reasonable way to visually represent the upgrade. I may revise the entire Aeon Land Factory model again. I’ve received feedback from multiple sources expressing the wish for a model that more closely resembles the original design from before the changes. Aeon Naval Factory – Hitbox / Effects I have not implemented effects of this kind in the project so far. However, I am still aware of the hitbox issue. If I decide to adjust the hitbox, I would also review other hitboxes as part of that process, since some of them do not fully match the updated model. DescriptionThe following hitboxes need adjustment, as they currently do not properly match their respective models: Affected Units Cybran Land Factory Seraphim Air Factory Aeon Naval Factory Expected Behavior Hitboxes accurately match the visible model dimensions. Consistent hitbox structure for units sharing the same base model. No regression of previously identified bugs. To Do Increase width of the Cybran Land Factory hitbox Apply T3 hitbox to all Seraphim Air Factory models Reduce Aeon Naval Factory hitbox size Perform regression testing for the known issue Below is a short overview along with images of the currently used hitboxes in the game. Lines marked in yellow indicate a deviation from the other tech levels of the same 
|
|
I'm currently reviewing and correcting all the blueprints and scripts. After that, I'll work on the hitboxes. Since I haven't quite understood Git yet, I would be grateful if someone could review this work afterwards. |
All Lua files in the factory models have been reviewed and revised. Changes: Removed unnecessary whitespace Adjusted path naming to match the common convention (fixed upper/lower case) Added the AverageDensity = parameter to support factories, as it is also present in all HQs Corrected incorrect health values Special Note: Since each factory variant is based on the T1 variant, the DDS files (Albedo, NormalTS, SpecTeam, LOD1, etc.) are referenced from there. Therefore, the files in the higher tech tiers are no longer required. If this commit does not remove those files, the core team may handle their removal after confirmation through testing by other contributors.
Here are the minor hitbox adjustments for various units. UEF Land T1 & T2 Increased SizeZ from 4.4 to 4.8 Air T3 Increased SizeZ from 3.5 to 4.2 AEON Air T1 Increased SizeX from 3 to 3.5 Increased SizeZ from 3 to 3.5 Naval T1 SizeZ = 13 This value remains unchanged because modifying it triggers a bug that shifts the build footprint (see first comment). The issue was narrowed down to the Physics section under: SkirtOffset SkirtSize It has now also been observed that the hitbox itself affects this issue. Since the build footprint can influence the faction behavior, this value remains unchanged for now until a better solution is found. Cybran Land T1, T2, T3 Increased SizeX from 2.9 to 4.3 Increased SizeZ from 4.2 to 4.3 Naval T3 SizeX = 4 This value was intended to be adjusted. However, since the model’s root bone is heavily aligned with the build cranes, a large empty space in front of the factory is included. Therefore, this value was not changed. Seraphim Air T1, T2, T3 Set SizeX to 3.5 Set SizeY to 2.7 Set SizeZ to 3.5 Since the model changes very little between tech levels, the hitbox has been standardized and averaged. Previously, the dimensions were partially too small or too tall. From my side, the project is now complete unless further issues are discovered.
|
Proposal: Adjustment of the Aeon Naval Hitbox The proposed adjustment should be carefully reviewed and evaluated. Purpose of the change The hitbox will be aligned more accurately with the existing model, ensuring better visual and technical consistency with the actual factory size. 
Potential impact The hitbox will more closely match the model. However, when multiple factories are placed in a row, their build position may shift after upgrading to the next tech level. The factories remain fully functional. There is a possibility of model overlap if units were placed before upgrading to the next tech level. 
Detailed Changes Aeon Naval T1, T2, T3 T1 SkirtOffsetZ: -1 → -2 SizeZ: 13 → 10.5 T2 SkirtOffsetZ: -1 → -2 SizeZ: 13 → 12 T3 SkirtOffsetZ: -1 → 0 SizeZ: 13 → 14 The current state of the project only presents the possibility that the hitbox of the T1 version is slightly too large. However, all other errors have been corrected. |
+ AI TargetBones +Hitbox SizeY =1
Target bone adjustment - B08 to B29 - Supplement B01
Minor adjustment to the size of the water effect, as this has partially disappeared under the new model.
3 participants
### Redesign of all HQ and Support Factories 2.0
As described in the forum post (https://forum.faforever.com/topic/5790/redesign-of-all-hq-and-support-factories), the first step towards redesigning the factory models was started some time ago. Since this work was unfortunately never completed, I have picked up the project again and continued it.
Overview of Changes
All models were recreated and adjusted based on the FAF templates, enabling upgrades to HQ and Support factories
All models now have their own build animations
LOD1 models were largely adjusted to match the new LOD0 models
### Specific Changes
Build Area of Naval Factories
The required build area for all naval factories has been adjusted.
This change is based on the observation that when an existing factory was upgraded to a higher tech level, the build footprint shifted once the mesh expanded.
This adjustment prevents this issue/effect from occurring (see forum post
https://forum.faforever.com/topic/5790/redesign-of-all-hq-and-support-factories/26).
Visual Adjustments to Aeon T3 Naval Ship Construction
As described in forum post (19), the Mercury Pool visuals were displayed disproportionately large compared to the ships.
These visuals have now been adjusted to better match the ship sizes (https://forum.faforever.com/topic/5790/redesign-of-all-hq-and-support-factories/63).
Added Death Animation for Seraphim Naval Factories
While creating the death animations for the Seraphim faction, it was discovered that the animations were not being played.
This issue was fixed by adding the PlayAnimation(...) function to the relevant scripts.
See: (https://forum.faforever.com/topic/5790/redesign-of-all-hq-and-support-factories/90)
Adjustment of TargetBones for Land and Air Factories
During testing, I noticed that some units were unable to damage certain factories as intended. Since no changes have been made to the hitboxes so far, this issue is likely caused by the reorientation of various bones in the updated models.
To verify this assumption, several tests were conducted with modified configurations. As a result, the UEF, Aeon, and Cybran factory models now use the following configuration:
AI = {
TargetBones = {
"Attachpoint",
},
},
I would suggest revising the hitboxes of the land and air factories. Since this issue was only briefly mentioned in the forum post, here are a few images to illustrate the point more clearly.