Skip to content
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

Ensure m3d faces in non-decreasing materialid sequence #3385

Merged
merged 1 commit into from
Oct 9, 2023

Conversation

orcmid
Copy link
Contributor

@orcmid orcmid commented Oct 7, 2023

This modification replaces the expensive qsort protection with an insertion sort that is near-instantaneous in the expected ordered case.

The changes desk-check and they compile. It is not confirmed in a running application and I am guessing that I have satisfied raylib coding style.

PS: This is modeled on Donald Knuth's Algorithm 5.2.1S (Straight insertion sort) and is a refinement of that and the code @DaveH355 provided earlier in order to handle mostly-ordered faces as quickly as possible.

This modification replaces the expensive qsort protection with an
insertion sort that is near-instantaneous in the expected ordered case.
@orcmid
Copy link
Contributor Author

orcmid commented Oct 8, 2023

I should add that this defensive approach is consistent with the steps taken a few lines below where this modification is inserted:

 // there should be only one material switch per material kind, but be bulletproof for non-optimal model files

@raysan5
Copy link
Owner

raysan5 commented Oct 8, 2023

@orcmid Thank you very much for the review but as stated in the M3D specs, this sorting is not needed any more. I prefer to avoid extra code for non-allowed side cases. In any case I can replace the qsort() statement with this code but it should remain commented, only for reference.

@orcmid
Copy link
Contributor Author

orcmid commented Oct 8, 2023

I think most folks are very reluctant to maintain a patched raylib, and only a small subset will even read this, so commented-out defenses don't seem particularly helpful to me.

I'm not going to argue. It's your project.

@raysan5 raysan5 merged commit 7ab911b into raysan5:master Oct 9, 2023
12 checks passed
@raysan5
Copy link
Owner

raysan5 commented Oct 9, 2023

As commented, I'm merging it for reference but keeping it commented. It is not required for as per the official M3D specs.

@raysan5
Copy link
Owner

raysan5 commented Oct 9, 2023

Side note, raylib does not use the do { } while approach anywhere in the library.

@orcmid
Copy link
Contributor Author

orcmid commented Oct 9, 2023

@raysan5

Side note, raylib does not use the do { } while approach anywhere in the library.

In this particular case it works perfectly along with rapid checking for already-sorted elements. I confess that I had to look it up to ensure I was using it correctly :).

@orcmid orcmid deleted the m3d-faces-check branch November 18, 2023 17:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants