Skip to content

Conversation

@Pirulax
Copy link
Contributor

@Pirulax Pirulax commented May 22, 2020

I've talked with Qais, and he said it would be a big help to refactor all these new functions.

Also, the not done list:

  • dxDrawPrimitive3D / dxDrawMaterialPrimitive3D - hard to read table structure. Its pretty specific, so i wont bother trying it with the new parser, will just add a big overhead.
  • engineGetModelTextures - performance issue(returning std::unordred_map, with possibly a lot of strings: a lot of HEAP allocs, which would kill the performance. I suggest to implement std::vector<std::pair<const char*, Type*>>, or with string_view.
  • SetColShapeSize - argument types depends on colshape type - impossible to do with the new parser(well, not impossible, but im not sure what would be the best way, ill leave it up to you)
  • GetColPolygonPoints - modifying it would kill the performance because of std::vector heap allocation on return
  • Add possibility of modifying dynamic objects behavior (#784)

Also, maybe its not the best place to discuss: I think adding the ability to return std::optional to the new parser would be dope. Theres a function in the col shape file, where I've left a TODO comment, because its ugly and we need a refactor.

@Pirulax
Copy link
Contributor Author

Pirulax commented May 22, 2020

I know there are a few messed up commits. Im sorry.

@Pirulax
Copy link
Contributor Author

Pirulax commented May 22, 2020

Seems like we either move
{"resetBlurLevel", CLuaDefs::ArgumentParser<CLuaFunctionDefs::ResetBlurLevel>}, to LuaDefs.World(Somehow, because CLuaManager loads all the defs from where) or we make CLuaDefs::ArgumentParser functions public or we do a private CLuaDefs to CLuaManager which would be dumb.

@Pirulax
Copy link
Contributor Author

Pirulax commented May 22, 2020

I also refactored the non OOP functions in ColShapeDefs to return numbers instead of vector(s)

@StrixG StrixG added this to the 1.5.8 milestone May 22, 2020
@Pirulax
Copy link
Contributor Author

Pirulax commented May 22, 2020

There you go.
The build doesnt compile.
I need someone who could answer my previous question,

Seems like we either move
{"resetBlurLevel", CLuaDefs::ArgumentParserCLuaFunctionDefs::ResetBlurLevel}, to LuaDefs.World(Somehow, because CLuaManager loads all the defs from where) or we make CLuaDefs::ArgumentParser functions public or we do a private CLuaDefs to CLuaManager which would be dumb.

also, DxDrawWiredSphere has const SColorARGB color, how should we deal with that?

@qaisjp qaisjp self-requested a review May 23, 2020 01:03
@Pirulax Pirulax force-pushed the refactor/newFuncsToNewParser branch 2 times, most recently from 240b8d3 to 2e2c68d Compare May 23, 2020 20:41
Copy link
Contributor

@qaisjp qaisjp left a comment

Choose a reason for hiding this comment

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

Annotated each suggested change with the name of the commit it should be melded into, so try and batch matching names if you accept the changes.

@Pirulax
Copy link
Contributor Author

Pirulax commented May 24, 2020

#1454 (comment)
@qaisjp
I cant comment on the issue, but no, its I've deleted the old function, and added the new, one liner(CStaticFDs was always returning true, so there was no need for that check)

@Pirulax
Copy link
Contributor Author

Pirulax commented May 24, 2020

Its still not compiling because we need to resolve the dxDrawWiredSphere argument SColorARGB. What should we do with that? Should I revert that commit? Or we implement SColorARGB?

@sbx320 I think I need your opinion here as well. I've asked about it before, but we didnt resolve it.

@qaisjp
Copy link
Contributor

qaisjp commented May 24, 2020

Implement SColor in a separate PR the same way the old parser works.

@Pirulax Pirulax force-pushed the refactor/newFuncsToNewParser branch 2 times, most recently from e437363 to abbaf65 Compare May 26, 2020 05:23
@Pirulax
Copy link
Contributor Author

Pirulax commented May 26, 2020

Alright, alright, there we go.
I did it this time.

@qaisjp
Copy link
Contributor

qaisjp commented May 26, 2020

Also, I didn't mention it earlier when I worked with you on the rebase since I didn't want to spend too much time with you watching rewords, but you need to reword every commit to be more clear.

"Refactor X" is not clear enough - someone must be able to read the commit message and know exactly what change was made.

It should be "Refactor X to use new Lua arg parser", and if there are other changes, include those in the commit descriptions.

@qaisjp qaisjp closed this May 26, 2020
@qaisjp qaisjp reopened this May 26, 2020
@Pirulax
Copy link
Contributor Author

Pirulax commented May 26, 2020

Okay, no problem.
Ill do it tomorrow.

@Pirulax Pirulax force-pushed the refactor/newFuncsToNewParser branch from abbaf65 to 964af05 Compare May 29, 2020 04:10
@Pirulax
Copy link
Contributor Author

Pirulax commented May 29, 2020

3261f1f, 6ed8668, 629e84e are messed up. I'm not quite sure how to fix it. I've googled it up, but the instructions on StackOverflow aren't totally clear to me, so if you could please explain it to me how could I change the contents of those commits, so they aren't totally messed up?

What I have in mind is to just cherry pick those 3 commits into a separate branch, there you can then separately commit each change (there a feature in VSC where you can select the lines, and make a commit out of the selected lines), and then just merge that branch back into this one.

if that makes sense.

@Pirulax
Copy link
Contributor Author

Pirulax commented May 29, 2020

Actually i could just go to another branch, squash merge the whole branch, and stage-commit changes line-by-line, so every commit would be perfect.

If theres a way to merge without having a 'merged branch..' commit automatically added?

@Pirulax
Copy link
Contributor Author

Pirulax commented May 29, 2020

Im not quite sure whats up with the new parser?
Please take a look at the build logs

@Pirulax
Copy link
Contributor Author

Pirulax commented Jun 3, 2020

All refactors have been moved to other PRs.

@qaisjp qaisjp removed this from the 1.5.8 milestone Jun 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants