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

Minor change #2342

Closed
wants to merge 1 commit into from
Closed

Minor change #2342

wants to merge 1 commit into from

Conversation

Zangetsu38
Copy link
Contributor

@Zangetsu38 Zangetsu38 commented Feb 5, 2017

Thx @kd-11 for help and approve this
d3d12: Fix typo, invalid no says Unknown it is a different and little clean up in D3D12Formats.
PPU: Fix two Warning compile.
Add info in BufferUtils for log.

@mention-bot
Copy link

@Zangetsu38, thanks for your PR! By analyzing the history of the files in this pull request, we identified @vlj, @Nekotekina and @raven02 to be potential reviewers.

@Nekotekina
Copy link
Member

What a useless PR. Stop wasting my time or I'll have to ban you again.

@O1L
Copy link
Contributor

O1L commented Feb 5, 2017

Actually we use only D3D (part of DirectX).
In truth, these changes do not do anything.

@Zangetsu38
Copy link
Contributor Author

Zangetsu38 commented Feb 5, 2017

I have only restore Unknown and DirectX 12 name for same other GCM_ENUM etc.., and same before and GSframe indicate DirectX 12 while setting dialog indicate D3D12, just more clear, that all,
And commit Allows Kung fuu Rider launch, just after this waiting kd-11 implement quad_strip.
And i have ask for sur before request PR, And I was told this little change it is interesting :/
With a light cleaning of the 3d12 format that contained a can some unnecessary line.
It is also the change in the bufferutil that has made it possible to find the missing value, well for that one, my said it was interested also, since it could have another one.
I'm just going to help the project, that's all.

@kd-11
Copy link
Contributor

kd-11 commented Feb 5, 2017

@Zangetsu38 As I mentioned earlier, its best to leave those cosmetic changes out and maybe submit only the stubbed cellMusic function and the BufferUtils improvement. Comsetic changes like that tend to be divisive and don't add new functionality.

@MrSapps
Copy link

MrSapps commented Feb 5, 2017

Seems strange that cellMusicSetVolume2 takes no arguments? If it does have arguments won't this unbalance the stack?

@Zangetsu38
Copy link
Contributor Author

@kd-11 Yes I know that, but the other 2 changes, are just the one for the useless code that had been forgotten, and make it all clearer, nothing else: /
And simply with a coherence between the settting dialog which says D3d12 and the window GSFRAME which indicates it DirectX 12.
It's just to be coherent, and it changes nothing.

@Nekotekina
Copy link
Member

Nekotekina commented Feb 5, 2017

Then change GUI back to D3D12.
And I'd merge it with only cellMusic change.

@Zangetsu38
Copy link
Contributor Author

Zangetsu38 commented Feb 5, 2017

@Nekotekina For Gui i have juste restored same before and also same in GSFrame, and also same Vk Named Vulkan and GL named OpenGL , and for D3D12 same think abbreviation for Direct3D12 = DirectX 12, It just seemed logical to me, it makes it a little strange caused by Only DirectX 12 use abbreviation, two other that well their full name, and name D3D12 no clear for all user
But with only cellmusic change ?
Also kd say Bufferutil improvement it is needed.

@Zangetsu38
Copy link
Contributor Author

I've done

PPU: Fix two Warning compile.
Add info in BufferUtils for log.
@Nekotekina
Copy link
Member

Oops. there is nothing to merge. Also I don't think that "Unknown" is better.

@Nekotekina Nekotekina closed this Feb 6, 2017
@Zangetsu38
Copy link
Contributor Author

@Nekotekina Sorry but precisely if we need it, kd-11 said it himself, the change in BuferUtil was necessary, I found the missing value to fix a games like that, and kd-11 will implement it after , We need it to find other value and therefore why he said we should merge.

And for Unknow, I only handed it over as it was before and especially as is the case in the rest of the code, it's only to be consistent with all the code, see here:
https://github.com/RPCS3/rpcs3/blob/master/rpcs3/Emu/RSX/gcm_enums.cpp#L16
https://github.com/RPCS3/rpcs3/blob/master/rpcs3/Emu/RSX/VK/VKGSRender.cpp#L20

This is already written Unknown in all the rest of the code, precisely because that is what it means, unknown value (not known therefore), value invalid means nothing in this case.

And I had not just done that, there was also a small cleanup of old undeleted code that had been forgotten at the time, "break" and "unsupported" are not useful if all boxes return a value , It was precisely in order to save you time.

@kd-11
Copy link
Contributor

kd-11 commented Feb 6, 2017

There are some unimplemented exapansions in BufferUtils which is why I agreed that the change was useful in that case. The gramatical stuff I don't care for.
The other important bits were commited with #2343

@Zangetsu38
Copy link
Contributor Author

@kd-11 Yes I know, I had to implement a single value in cell music, since I did not test the rest, otherwise I would have done too.
But I have not just renamed the "invalid", there is also some old useless code cleanup that has been forgotten.
But so I have to do what, remake a PR with only the change in buffertutil?

@kd-11
Copy link
Contributor

kd-11 commented Feb 6, 2017

That change alone would be too small to justify a PR by itself. While it would provide some useful information in case of a crash, it does not improve functionality, which is why I usually let my work stay in my private fork for a while until I have something bigger to provide.
I will include the improvement when I submit my buffer synchronization work.

@Zangetsu38
Copy link
Contributor Author

ho ok, i see sorry :/

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.

None yet

6 participants