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

Ogre 2.1 #76

Closed
wants to merge 19 commits into from
Closed

Ogre 2.1 #76

wants to merge 19 commits into from

Conversation

DotWolff
Copy link

Support for ogre 2.x using OpenGL3.x+ and DirectX11.
Check option 8 in platforms for Ogre 2.x.

http://www.ogre3d.org/forums/viewtopic.php?f=25&t=83569

@ghost
Copy link

ghost commented Jun 12, 2015

I am not very familiar with Ogre 2.1, so unfortunately not in the best position to review this. Perhaps we could ask the Ogre community to provide some testing and feedback?

I'm thinking the new rendersystem should be named Ogre21 platform, since it only supports 2.1 and not 2.0, correct? We already have a Ogre 2.0 rendersystem in a separate branch.

@aymarfisherman
Copy link

Hey scrawl, I'm part of the team who made the port, we'll change the platform name to Ogre21Platform as requested, also we posted a thread on Ogre forums, hopefully someone there will help review the pull req.

We tested it here with the raw Ogre 2.1 branch (just initialized MyGUI in one of the samples)and the results are the following: we tested simple widgets; widgets composed of multiple sub widgets; font rendering; and scissor test. All of those worked perfectly, exactly how they work with the OgrePlatform from MyGUI master and Ogre 1.9 / 1.10.

Also, we made an effort to leave the Ogre21Platform code style in accordance with the MyGUI code style. And we edited the CMake already to leave everything as easy as it can be to build and use the new platform.

Let us know if there's anything else we can do to move forward the review process. :)

@ghost
Copy link

ghost commented Jun 14, 2015

The changes to FilterNoneSkin break the build when compiling with the Ogre 1.x platform. See the travis build log.

@aymarfisherman
Copy link

That file wasn't supposed to be changed, our bad, we'll fix it tomorrow.

al2950 and others added 3 commits June 14, 2015 23:36
@al2950
Copy link

al2950 commented Jun 18, 2015

FYI, I have looked at the code and there are a couple of things I have recommended,
http://ogre3d.org/forums/viewtopic.php?f=25&t=83569#p518382
in fact I have already done them and just need to merge changes with this fork. I hope to get this tested and pushed by this weekend

@al2950
Copy link

al2950 commented Jun 20, 2015

I have to disagree with the rename of Ogre2Platform to Ogre21Platform. I appreciate there is already an Ogre 2 version in a separate branch, but thats not really going to be used. We can easily add an error message in Cmake that warns if not using >=2.1.

This issue is that it supports anything above Ogre 2.1, and you dont want to have to change the cmake scripts every new Ogre version, or add a new platform for that matter!

@Altren
Copy link
Contributor

Altren commented Jun 21, 2015

I agree, that having 2.1 name might be bad idea.
I guess that main reason for such naming is that it doesn't work with Ogre 2.0, but this might be better to skip Ogre2.0 platfrom, or if it needed alot name it Ogre20Platform instead and Ogre2Platform for 2.1+ versions.

@aymarfisherman
Copy link

Makes sense, since Ogre 2.0 won't even be released as stable.

Should we rename it back to Ogre2Platform scrawl?

@ghost
Copy link

ghost commented Jun 24, 2015

I agree, that having 2.1 name might be bad idea.
I guess that main reason for such naming is that it doesn't work with Ogre 2.0, but this might be better to skip Ogre2.0 platfrom, or if it needed alot name it Ogre20Platform instead and Ogre2Platform for 2.1+ versions.

Makes sense, since Ogre 2.0 won't even be released as stable.

True. And I'm starting to think that having 2.0 and 2.1 platforms in the main branch at this point might not be a good idea after all.

This is how I see it:

  • Code duplication. There are lot of differences with the versions, but also lot of similarities (resource manager, etc. Having those code duplicated seems like a bad idea maintenance wise.
  • Testing is difficult. Ogre 2.0, 2.1 and 1.x can be installed to the same prefix, but the FindOgre script doesn't allow setting the desired version.
  • 2.1 and 2.0 aren't released stable yet. Do we want MyGUI platforms using unreleased software in a MyGUI release? I think the answer should be no.

IMO, we should merge your work to a new ogre2.1 branch and keep it there, until there is a stable release. Just fix the merge conflicts and we should be good to go.

…ng added during renderQueueStarted

- Please note the custom compositor pass is effectively grafted from Scrawl's Ogre2 branch
@al2950
Copy link

al2950 commented Jun 24, 2015

I agree, backout the rename changset and for now just have a separate branch for Ogre 2.1 as it is not 'stable'. I have a number of changes that can be found here;
https://github.com/al2950/mygui/commits/master
This fixes a few issues, and integrates scrawl's custom pass method. I just need to test the cmake changes still work with Ogre 1.x and then I will create pull request to DotWolff's fork.

@al2950
Copy link

al2950 commented Jun 24, 2015

On a side note, how do you request a pull request to go into a new branch in github!?

@ghost
Copy link

ghost commented Jun 24, 2015

On a side note, how do you request a pull request to go into a new branch in github!?

You can't, AFAIK.

@aymarfisherman
Copy link

Hey al2950,

We are renaming the platform today, can you create your pull request so we can merge in our fork?

Also, scrawl, we discovered we can change the base our fork is using, so we can actually set the pull request to go into any branch, what I need is a clean Ogre2.1 branch in MyGUI rep so we can redirect our fork to it and create a new pull request.

@ghost
Copy link

ghost commented Jun 25, 2015

what I need is a clean Ogre2.1 branch in MyGUI rep so we can redirect our fork to it and create a new pull request.

Done.

@al2950
Copy link

al2950 commented Jun 25, 2015

Hey al2950,

We are renaming the platform today, can you create your pull request so we can merge in our fork?

I have just noticed another issue regarding how the dummy movable object is created, this needs to be fixed but is not an immediate issue. Would you like me to create a pull request on what I have done, or are you happy to wait until I have sorted it?

@aymarfisherman
Copy link

We can wait, let us know once you finish it, meanwhile we'll fix the merge conflicts.

…r which means it would be processed as if it was a scene object on every pass.

-Fixed minor memory leak
@al2950
Copy link

al2950 commented Jun 29, 2015

@aymarfisherman

Please can you reverse the 'Renamed to Ogre21Plataform' commit?
( 559be0f )

NB I have fixed the MyGUI_FilterNoneSkin.cpp issues in my local fork

It causes too many headaches when merging with my stuff, and I dont get GIT as every time I try and revert the commit in my local repo the revert commit disappears when changing back to my branch. I really dont understand why so many people prefer GIT over Hg!!

@aymarfisherman
Copy link

@al2950 It's done, we renamed everything back to Ogre2. We didn't revert the commit because there were files that weren't named correctly before that commit.

Conflicts:
	CMake/Utils/MyGUIConfigTargets.cmake
Conflicts:
	CMake/Utils/MyGUIConfigTargets.cmake
@al2950
Copy link

al2950 commented Jul 8, 2015

I created a pull request a few days ago to DotWolff fork , but seems no activity. I could create a pull request directly to this fork if people wanted?

MyGUI Ogre 2.1 updates
@al2950
Copy link

al2950 commented Jul 10, 2015

Could you create a new pull request to pull these changes into the new Ogre2.1 branch instead of the master? (Or edit this one)

@aymarfisherman
Copy link

New Pull Request created:
#86

@Altren
Copy link
Contributor

Altren commented Jul 21, 2015

Can we close this pull request, since #86 was accepted?

@ghost ghost closed this Jul 21, 2015
This pull request was closed.
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

4 participants